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

Conversation

clydin
Copy link
Member

@clydin clydin commented Jul 18, 2018

@clydin clydin requested a review from hansl July 18, 2018 00:39
@clydin clydin force-pushed the full-paths-plugin branch 4 times, most recently from 1da3017 to fbbeb80 Compare July 18, 2018 02:00
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

That's a huge change just before release. I'd rather just revert the original buggy PR and investigate for 6.2.

@@ -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.

@clydin
Copy link
Member Author

clydin commented Jul 18, 2018

Can hold off for 6.2 or even 7.0, if prefered. This also cleans up and fixes several other behavioral discrepancies in the process.

// 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).

@clydin clydin added target: major This PR is targeted for the next major release and removed target: minor labels Aug 16, 2018
@clydin clydin force-pushed the full-paths-plugin branch from 12a8244 to bbf1a02 Compare August 20, 2018 16:17
@ngbot
Copy link

ngbot bot commented Sep 7, 2018

Hi @clydin! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@hansl hansl removed the cla: yes label Sep 26, 2018
@hansl hansl merged commit be6f757 into angular:master Sep 26, 2018
@clydin clydin deleted the full-paths-plugin branch September 26, 2018 14:40
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
4 participants