fix(@ngtools/webpack): validate wildcard path replacements

[jonrimmer]

As described in this angular-cli issue, the logic for handling wildcard paths does not match how TypeScript works, and breaks non-relative module imports that happen to match a wildcard path.

The module resolution docs explain how TypeScript considers each mapping in turn as a way to potentially turn a non-relative import into a relative one. However, this mapping is used if and only if the resulting path actually matches a file that exists on disk. If an import matches a path, but does not resolve to a real file, TypeScript will fall back to regular node module resolution.

For example, if I have a path mapping "*": ["./src/app"], then this will match any import, including an import of '@angular/core'. As such, TypeScript will initially try to resolve this as a file with the path {baseUrl}/src/@angular/core. However, when it cannot find a file in this location, it will fall back to regular resolution, likely loading the module from {baseUrl}/node_modules/@angular/core.

As it stands, the paths plugin does not validate whether the mapped file exists. It assumes it does and passes it on to Webpack without checking. The result is that "*" wildcard mappings, of the type given in the TypeScript documentation, will break things, because they cause the plugin to transform non-relative module imports into invalid, relative mappings.

This change attempts to fix the problem by validating whether the result of applying a mapping matches a real file. It considers two possibilities:

1. The mapped path exactly matches a file on disk. E.g. an import of style/something.css, where there is a file {baseUrl}/src/app/style/something.css.
2. The mapped path is an extensionless import that TypeScript can resolve. E.g. an import of core/utils, where there is a file {baseUrl}/src/app/core/utils.ts (or .tsx, etc.).

Even with this change, things probably still don't fully match how TypeScript handles path mappings, but it should be considerably closer.

The guidelines require fixes to be covered by tests, but as there doesn't seem to be any existing tests for the entire plugin, and I'm not too sure how to approach writing one for this, given its dependencies on the filesystem and the TypeScript compiler. If somebody can provide some suggestions on how to test this, I'd be happy to apply them.

[clydin]

I will concede and give the benefit of the doubt on the potentially aspect as the behavior does appear to differ from the, albeit, ambiguous documentation (intentionally or otherwise). A formalized module resolution specification would be useful in this case.

However, as discussed in my comment here. This implementation change will break far more than it actually fixes. A deeper integration into Webpack's module resolution system would be necessary to properly implement the suggested design changes.

[jonrimmer]

What do you mean "will break far more than it actually fixes"? The current code is broken for the paths example given in the TypeScript documentation! A "*"mapping pointing to the src folder is the single most basic use-case of path mappings, and even a cursory search of GitHub reveals many projects using this construction. What empirical reason do you have to think that leaving this broken will be less harmful than fixing it, even if the fix is imperfect?

And I don't understand your objection to using resolveModuleName? The issues you describe in the linked issue relate to the resolution of non-relative module names. It's true that, for a non-relative module, TypeScript could incorrectly think it exists when it doesn't by considering typings and such like. But my code is using it to check the resolution of the mapped replacement, which is the result of a path.resolve() call, and will be relative to the baseUrl. In this case, TS resolution will be far more constrained, essentially just looking for a matching file with a supported extension at that path.

Can you provide an example of a TS import that currently works correctly, which you think would be broken after applying this change?

[clydin]

A star mapping pointing to the src folder is unnecessary. baseUrl handles that case already.

As mentioned in the linked comment, ts.resolveModuleName attempts to resolve for type information not necessarily the underlying source code needed to bundle the application. Accurately resolving the actual source code from the path of a .d.ts file is not possible in all cases. It also does not respect the alternate package.json entry points required for proper bundling (i.e., esm5, esm2015, and browser). It is therefore not useful in this scenario and its use will be completely removed in the 6.x timeframe.

[clydin]

If you are interested in working on this, a Webpack resolver plugin that fully implements the path mapping algorithm (which is mostly complete in the existing code) would be needed. But would need to iterate through all the match potentials for the particular chosen path pattern. Some examples would be this or the others here.

[jonrimmer]

OK, I'll take a look.

[alexeagle]

Sorry @jonrimmer - this got lost when we moved back to the angular/angular-cli repo. If you still think this pull request is relevant, could you please re-base on that repo and open a new PR there? Thanks and sorry for the extra churn.