fix(ivy): ngcc - prefer JavaScript source files when resolving module… #30017

JoostK · 2019-04-21T19:32:05Z

… imports

Packages that do not follow APF may have the declaration files in the same
directory as one source format, typically ES5. This is problematic for ngcc,
as it needs to create a TypeScript program with all JavaScript sources of
an entry-point, whereas TypeScript's module resolution mechanism would have
resolved an internal module import to the external facing .d.ts declaration
file, instead of the JavaScript source file. This behavior results in the
program to be analysed being incomplete.

This commit introduces a custom compiler host that recognizes the above
scenario and rewires the resolution of a .d.ts declaration file to its
JavaScript counterpart, if applicable.

Fixes #29939

alxhub · 2019-04-22T17:24:47Z

packages/compiler-cli/ngcc/src/packages/entry_point_bundle.ts

+      host.getCurrentDirectory(), file => host.getCanonicalFileName(file));
+  host.resolveModuleNames = (moduleNames, containingFile, reusedNames, redirectedReference) => {
+    return moduleNames.map(moduleName => {
+      const {resolvedModule} = ts.resolveModuleName(


I know @petebacondarwin ran into some issues trying to use this function, with how terribly it performed. I'm guessing the ts.ModuleResolutionCache mitigates that considerably. I actually imagine TS has a similar implementation internally.

@petebacondarwin, do you have any perf concerns here? Otherwise this LGTM.

This is almost the same code as what TS uses by default, with one difference: TypeScript's default logic has an additional caching layer, besides the module resolution cache:

See loadWithLocalCache:
https://github.com/Microsoft/TypeScript/blob/b534fb4849eca0a792199fb6c0cb8849fece1cfd/src/compiler/program.ts#L409-L426

used here:
https://github.com/Microsoft/TypeScript/blob/b534fb4849eca0a792199fb6c0cb8849fece1cfd/src/compiler/program.ts#L653-L657

petebacondarwin · 2019-04-24T21:05:42Z

I think we should land #29643 and then consider using the NgccCompilerHost and the ModuleResolver to do this work.

MathewBerg · 2019-04-30T12:16:04Z

Until/if this PR lands, is there a suggested workaround that can be done locally? I'd love to test IVY but use angularfire2 extensively.

petebacondarwin · 2019-04-30T12:33:56Z

Now that #29643 has landed we should be able to move this PR along.

JoostK · 2019-04-30T21:26:02Z

I rebased this and derived a custom compiler host based on NgccCompilerHost. I did not use the ModuleResolver right now, as it introduces some things we would need to think about:

For ResolvedExternalModule we'd have to determine the typings location, i.e. load the package.json file and resolve its typings key. If it doesn't have it, we should instead use main (or others?) to resolve the .js source file instead?
For ResolvedDeepImport we also need to resolve the typings key in package.json and derive the correct .d.ts file use the deeply imported path and and the root of the typings directory.

I can't really foresee if the above would be at all accurate/cover our use-cases.

… imports Packages that do not follow APF may have the declaration files in the same directory as one source format, typically ES5. This is problematic for ngcc, as it needs to create a TypeScript program with all JavaScript sources of an entry-point, whereas TypeScript's module resolution mechanism would have resolved an internal module import to the external facing .d.ts declaration file, instead of the JavaScript source file. This behavior results in the program to be analysed being incomplete. This commit introduces a custom compiler host that recognizes the above scenario and rewires the resolution of a .d.ts declaration file to its JavaScript counterpart, if applicable. Fixes angular#29939

… module imports

googlebot · 2019-05-01T14:07:10Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

… module imports

Sometimes we need to override module resolution behaviour. We do this by implementing the optional method `resolveModuleNames()` on `CompilerHost`. This commit ensures that we always try this method first before falling back to the standard `ts.resolveModuleName`

ngbot · 2019-05-01T14:47:00Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "cla/google" is failing
     missing required labels: cla: yes
     forbidden labels detected: cla: no
     status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

petebacondarwin · 2019-05-01T14:55:20Z

Caretaker (merge-assistance): @JoostK and @petebacondarwin have boths signed CLAs.

googlebot · 2019-05-01T20:14:24Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

kara · 2019-05-01T20:15:36Z

presubmit

Sometimes we need to override module resolution behaviour. We do this by implementing the optional method `resolveModuleNames()` on `CompilerHost`. This commit ensures that we always try this method first before falling back to the standard `ts.resolveModuleName` PR Close #30017

… imports (angular#30017) Packages that do not follow APF may have the declaration files in the same directory as one source format, typically ES5. This is problematic for ngcc, as it needs to create a TypeScript program with all JavaScript sources of an entry-point, whereas TypeScript's module resolution mechanism would have resolved an internal module import to the external facing .d.ts declaration file, instead of the JavaScript source file. This behavior results in the program to be analysed being incomplete. This commit introduces a custom compiler host that recognizes the above scenario and rewires the resolution of a .d.ts declaration file to its JavaScript counterpart, if applicable. Fixes angular#29939 PR Close angular#30017

…ar#30017) Sometimes we need to override module resolution behaviour. We do this by implementing the optional method `resolveModuleNames()` on `CompilerHost`. This commit ensures that we always try this method first before falling back to the standard `ts.resolveModuleName` PR Close angular#30017

angular-automatic-lock-bot · 2019-09-15T00:53:21Z

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

JoostK added type: bug/fix severity3: broken target: major This PR is targeted for the next major release risk: low labels Apr 21, 2019

ngbot bot added this to the needsTriage milestone Apr 21, 2019

googlebot added the cla: yes label Apr 21, 2019

JoostK mentioned this pull request Apr 21, 2019

Ivy fails with firebase #29939

Closed

JoostK force-pushed the ngcc-source-imports branch 2 times, most recently from e479741 to 69b2f33 Compare April 21, 2019 20:04

JoostK marked this pull request as ready for review April 21, 2019 20:53

JoostK requested a review from a team as a code owner April 21, 2019 20:53

JoostK mentioned this pull request Apr 22, 2019

fix(ivy): ngcc - do not copy declaration files into bundle clone #30020

Closed

kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 24, 2019

alxhub approved these changes Apr 24, 2019

View reviewed changes

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 24, 2019

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Apr 24, 2019

kara assigned JoostK Apr 24, 2019

JoostK added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Apr 27, 2019

JoostK force-pushed the ngcc-source-imports branch from 69b2f33 to cb5f2fa Compare April 30, 2019 21:13

JoostK added 2 commits April 30, 2019 23:29

fixup! fix(ivy): ngcc - prefer JavaScript source files when resolving…

a39c404

… module imports

JoostK force-pushed the ngcc-source-imports branch from cb5f2fa to a39c404 Compare April 30, 2019 21:30

petebacondarwin added target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 1, 2019

JoostK requested a review from a team as a code owner May 1, 2019 14:06

googlebot added cla: no and removed cla: yes labels May 1, 2019

petebacondarwin added 2 commits May 1, 2019 15:12

fixup! fix(ivy): ngcc - prefer JavaScript source files when resolving…

8d91bb5

… module imports

petebacondarwin force-pushed the ngcc-source-imports branch from 951fe43 to a0d356d Compare May 1, 2019 14:18

petebacondarwin approved these changes May 1, 2019

View reviewed changes

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 1, 2019

alxhub approved these changes May 1, 2019

View reviewed changes

petebacondarwin added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 1, 2019

petebacondarwin mentioned this pull request May 1, 2019

ngcc: CommonJS support #30200

Closed

kara added cla: yes and removed cla: no labels May 1, 2019

kara added the action: presubmit The PR is in need of a google3 presubmit label May 1, 2019

kara unassigned JoostK May 1, 2019

kara removed the action: presubmit The PR is in need of a google3 presubmit label May 1, 2019

kara closed this in 638ba4a May 1, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): ngcc - prefer JavaScript source files when resolving module… #30017

fix(ivy): ngcc - prefer JavaScript source files when resolving module… #30017

JoostK commented Apr 21, 2019

alxhub Apr 22, 2019

JoostK Apr 24, 2019

petebacondarwin commented Apr 24, 2019

MathewBerg commented Apr 30, 2019

petebacondarwin commented Apr 30, 2019

JoostK commented Apr 30, 2019

googlebot commented May 1, 2019

ngbot bot commented May 1, 2019

petebacondarwin commented May 1, 2019

googlebot commented May 1, 2019

kara commented May 1, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

fix(ivy): ngcc - prefer JavaScript source files when resolving module… #30017

fix(ivy): ngcc - prefer JavaScript source files when resolving module… #30017

Conversation

JoostK commented Apr 21, 2019

alxhub Apr 22, 2019

Choose a reason for hiding this comment

JoostK Apr 24, 2019

Choose a reason for hiding this comment

petebacondarwin commented Apr 24, 2019

MathewBerg commented Apr 30, 2019

petebacondarwin commented Apr 30, 2019

JoostK commented Apr 30, 2019

googlebot commented May 1, 2019

ngbot bot commented May 1, 2019

petebacondarwin commented May 1, 2019

googlebot commented May 1, 2019

kara commented May 1, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019