Skip to content

refactor(@ngtools/webpack): use webpack resolver plugin for path mapping #11575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/ngtools/webpack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"homepage": "https://github.com/angular/angular-cli/tree/master/packages/@ngtools/webpack",
"dependencies": {
"@angular-devkit/core": "0.0.0",
"enhanced-resolve": "4.1.0",
"rxjs": "6.2.2",
"tree-kill": "1.2.0",
"webpack-sources": "1.2.0"
Expand Down
25 changes: 9 additions & 16 deletions packages/ngtools/webpack/src/angular_compiler_plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { WebpackCompilerHost, workaroundResolve } from './compiler_host';
import { resolveEntryModuleFromMain } from './entry_resolver';
import { gatherDiagnostics, hasErrors } from './gather_diagnostics';
import { LazyRouteMap, findLazyRoutes } from './lazy_routes';
import { resolveWithPaths } from './paths-plugin';
import { TypeScriptPathsPlugin } from './paths-plugin';
import { WebpackResourceLoader } from './resource_loader';
import {
exportLazyModuleMap,
Expand Down Expand Up @@ -729,6 +729,14 @@ export class AngularCompilerPlugin {
});

compiler.hooks.afterResolvers.tap('angular-compiler', compiler => {
// tslint:disable-next-line:no-any
(compiler as any).resolverFactory.hooks.resolver
.for('normal')
// tslint:disable-next-line:no-any
.tap('angular-compiler', (resolver: any) => {
new TypeScriptPathsPlugin(this._compilerOptions).apply(resolver);
});

compiler.hooks.normalModuleFactory.tap('angular-compiler', nmf => {
// Virtual file system.
// TODO: consider if it's better to remove this plugin and instead make it wait on the
Expand All @@ -754,21 +762,6 @@ export class AngularCompilerPlugin {
);
});
});

compiler.hooks.normalModuleFactory.tap('angular-compiler', nmf => {
nmf.hooks.beforeResolve.tapAsync(
'angular-compiler',
(request: NormalModuleFactoryRequest, callback: Callback<NormalModuleFactoryRequest>) => {
resolveWithPaths(
request,
callback,
this._compilerOptions,
this._compilerHost,
this._moduleResolutionCache,
);
},
);
});
}

private async _make(compilation: compilation.Compilation) {
Expand Down
193 changes: 97 additions & 96 deletions packages/ngtools/webpack/src/paths-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,54 +6,97 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as path from 'path';
import * as ts from 'typescript';
import {
Callback,
NormalModuleFactoryRequest,
} from './webpack';


export function resolveWithPaths(
request: NormalModuleFactoryRequest,
callback: Callback<NormalModuleFactoryRequest>,
compilerOptions: ts.CompilerOptions,
host: ts.CompilerHost,
cache?: ts.ModuleResolutionCache,
) {
if (!request || !request.request || !compilerOptions.paths) {
callback(null, request);

return;
}
import { CompilerOptions, MapLike } from 'typescript';
import { NormalModuleFactoryRequest } from './webpack';

// Only work on Javascript/TypeScript issuers.
if (!request.contextInfo.issuer || !request.contextInfo.issuer.match(/\.[jt]sx?$/)) {
callback(null, request);
const getInnerRequest = require('enhanced-resolve/lib/getInnerRequest');

return;
}
export interface TypeScriptPathsPluginOptions extends Pick<CompilerOptions, 'paths' | 'baseUrl'> {

const originalRequest = request.request.trim();
}

// Relative requests are not mapped
if (originalRequest.startsWith('.') || originalRequest.startsWith('/')) {
callback(null, request);
export class TypeScriptPathsPlugin {
constructor(private _options: TypeScriptPathsPluginOptions) { }

return;
}

// Amd requests are not mapped
if (originalRequest.startsWith('!!webpack amd')) {
callback(null, request);
// tslint:disable-next-line:no-any
apply(resolver: any) {
if (!this._options.paths || Object.keys(this._options.paths).length === 0) {
return;
}

return;
const target = resolver.ensureHook('resolve');
const resolveAsync = (request: NormalModuleFactoryRequest, requestContext: {}) => {
return new Promise<NormalModuleFactoryRequest | undefined>((resolve, reject) => {
resolver.doResolve(
target,
request,
'',
requestContext,
(error: Error | null, result: NormalModuleFactoryRequest | undefined) => {
if (error) {
reject(error);
} else {
resolve(result);
}
},
);
});
};

resolver.getHook('described-resolve').tapPromise(
'TypeScriptPathsPlugin',
async (request: NormalModuleFactoryRequest, resolveContext: {}) => {
if (!request || request.typescriptPathMapped) {
return;
}

const originalRequest = getInnerRequest(resolver, request);
if (!originalRequest) {
return;
}

// Only work on Javascript/TypeScript issuers.
if (!request.context.issuer || !request.context.issuer.match(/\.[jt]sx?$/)) {
return;
}

// Relative or absolute requests are not mapped
if (originalRequest.startsWith('.') || originalRequest.startsWith('/')) {
return;
}

// Amd requests are not mapped
if (originalRequest.startsWith('!!webpack amd')) {
return;
}

const replacements = findReplacements(originalRequest, this._options.paths || {});
for (const potential of replacements) {
const potentialRequest = {
...request,
request: path.resolve(this._options.baseUrl || '', potential),
typescriptPathMapped: true,
};
const result = await resolveAsync(potentialRequest, resolveContext);

if (result) {
return result;
}
}
},
);
}
}

function findReplacements(
originalRequest: string,
paths: MapLike<string[]>,
): Iterable<string> {
// check if any path mapping rules are relevant
const pathMapOptions = [];
for (const pattern in compilerOptions.paths) {
for (const pattern in paths) {
// get potentials and remove duplicates; JS Set maintains insertion order
const potentials = Array.from(new Set(compilerOptions.paths[pattern]));
const potentials = Array.from(new Set(paths[pattern]));
if (potentials.length === 0) {
// no potential replacements so skip
continue;
Expand Down Expand Up @@ -96,9 +139,7 @@ export function resolveWithPaths(
}

if (pathMapOptions.length === 0) {
callback(null, request);

return;
return [];
}

// exact matches take priority then largest prefix match
Expand All @@ -112,63 +153,23 @@ export function resolveWithPaths(
}
});

if (pathMapOptions[0].potentials.length === 1) {
const onlyPotential = pathMapOptions[0].potentials[0];
let replacement;
const starIndex = onlyPotential.indexOf('*');
if (starIndex === -1) {
replacement = onlyPotential;
} else if (starIndex === onlyPotential.length - 1) {
replacement = onlyPotential.slice(0, -1) + pathMapOptions[0].partial;
} else {
const [prefix, suffix] = onlyPotential.split('*');
replacement = prefix + pathMapOptions[0].partial + suffix;
}

request.request = path.resolve(compilerOptions.baseUrl || '', replacement);
callback(null, request);

return;
}

// TODO: The following is used when there is more than one potential and will not be
// needed once this is turned into a full webpack resolver plugin

const moduleResolver = ts.resolveModuleName(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we give up on TypeScript resolution, which fixed a load of issues and supports tsconfig's path mapping. See #7473

Copy link
Contributor

@hansl hansl Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly curious of the two following assumptions in this PR:

  1. we implement TypeScript's resolution ourselves, without any bugs and edge cases, and
  2. we rely on TypeScript never updating their resolution algorithm and configurations, otherwise this will likely break.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another example is how we lose support for rootDirs, which I think we don't test for.

Copy link
Member Author

@clydin clydin Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rootDirs never really worked properly (if ever). It is also a fairly esoteric option and far more useful outside of bundling scenarios. However, we could also add support for it. The logic is very similar to the paths option but for relative imports.

#7473 actually demonstrates some of the issues with using resolveModuleName. A large and ever growing list of special processing is needed to attempt to determine and further locate the correct file to be bundled. As opposed to the correct file to provide type information for TypeScript transpilation.

Of note also is that the other two typescript loaders (ts-loader/awesome-typescript-loader) both take the same general approach of re-implementing the algorithm.
In regards to breakage, if TypeScript changed the algorithm or configuration options, it would result in a massive breaking change in regards to TypeScript projects in general. Failure of the TypeScript team to notify the user base of such a pending change in advance would result in a large percentage of existing TypeScript projects to fail. Advanced notification would mitigate the danger inherent in such a scenario.

Overall, the more ideal scenario would be if the underlying intrinsics for resolveModuleName within TypeScript were made public (someone alterations may also be necessary to allow for full re-usability; this would need further analysis). This could potentially allow this plugin to leverage more of the existing logic within TypeScript and limit the need to re-implement. However, the full Webpack resolver plugin approach would still be the appropriate implementation option as it allows multiple resolution attempts, the possibility of fallback to existing resolution, and integration into the webpack resolution pipeline (which could potentially contain additional custom resolution plugins).

originalRequest,
request.contextInfo.issuer,
compilerOptions,
host,
cache,
);

const moduleFilePath = moduleResolver.resolvedModule
&& moduleResolver.resolvedModule.resolvedFileName;

// If there is no result, let webpack try to resolve
if (!moduleFilePath) {
callback(null, request);

return;
}

// If TypeScript gives us a `.d.ts`, it is probably a node module
if (moduleFilePath.endsWith('.d.ts')) {
// If in a package, let webpack resolve the package
const packageRootPath = path.join(path.dirname(moduleFilePath), 'package.json');
if (!host.fileExists(packageRootPath)) {
// Otherwise, if there is a file with a .js extension use that
const jsFilePath = moduleFilePath.slice(0, -5) + '.js';
if (host.fileExists(jsFilePath)) {
request.request = jsFilePath;
const replacements: string[] = [];
pathMapOptions.forEach(option => {
for (const potential of option.potentials) {
let replacement;
const starIndex = potential.indexOf('*');
if (starIndex === -1) {
replacement = potential;
} else if (starIndex === potential.length - 1) {
replacement = potential.slice(0, -1) + option.partial;
} else {
const [prefix, suffix] = potential.split('*');
replacement = prefix + option.partial + suffix;
}
}

callback(null, request);

return;
}
replacements.push(replacement);
}
});

request.request = moduleFilePath;
callback(null, request);
return replacements;
}
2 changes: 2 additions & 0 deletions packages/ngtools/webpack/src/webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ export interface Callback<T = any> {

export interface NormalModuleFactoryRequest {
request: string;
context: { issuer: string };
contextInfo: { issuer: string };
typescriptPathMapped?: boolean;
}

export interface InputFileSystem {
Expand Down
4 changes: 2 additions & 2 deletions tests/legacy-cli/e2e/tests/misc/module-resolution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,14 @@ export default async function () {

await updateJsonFile('tsconfig.json', tsconfig => {
tsconfig.compilerOptions.paths = {
'@firebase/polyfill': ['@firebase/polyfill/index.ts'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on with these paths? It looks like breaking backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad test. They were failing due to a bug (which this PR fixes) rather then the intended reason. it should be failing due to non-project typescript files being pulled into the compilation which it now does. Before it was failing due to missing files.

'@firebase/polyfill': ['./node_modules/@firebase/polyfill/index.ts'],
};
});
await expectToFail(() => ng('build'));

await updateJsonFile('tsconfig.json', tsconfig => {
tsconfig.compilerOptions.paths = {
'@firebase/polyfill*': ['@firebase/polyfill/index.ts'],
'@firebase/polyfill*': ['./node_modules/@firebase/polyfill/index.ts'],
};
});
await expectToFail(() => ng('build'));
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2555,7 +2555,7 @@ engine.io@~3.2.0:
engine.io-parser "~2.1.0"
ws "~3.3.1"

enhanced-resolve@^4.1.0:
enhanced-resolve@4.1.0, enhanced-resolve@^4.1.0:
version "4.1.0"
resolved "https://registry.yarnpkg.com/enhanced-resolve/-/enhanced-resolve-4.1.0.tgz#41c7e0bfdfe74ac1ffe1e57ad6a5c6c9f3742a7f"
dependencies:
Expand Down