Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Apr 21, 2019

Previously, all of a program's files would be copied into the ivy_ngcc
folder where ngcc then writes its modifications into. The set of source files
in a program however is much larger than the source files contained within
the entry-point of interest, so many more files were copied than necessary.
Even worse, it may occur that an unrelated file in the program would collide
with an already existing source file, resulting in incorrectly overwriting
a file with unrelated content. This behavior has actually been observed
with @angular/animations and @angular/platform-browser/animations, where
the former package would overwrite declaration files of the latter package.

This commit fixes the issue by only copying relevant source files when cloning
a bundle's content into ivy_ngcc.

Fixes #29960

Previously, all of a program's files would be copied into the __ivy_ngcc__
folder where ngcc then writes its modifications into. The set of source files
in a program however is much larger than the source files contained within
the entry-point of interest, so many more files were copied than necessary.
Even worse, it may occur that an unrelated file in the program would collide
with an already existing source file, resulting in incorrectly overwriting
a file with unrelated content. This behavior has actually been observed
with @angular/animations and @angular/platform-browser/animations, where
the former package would overwrite declaration files of the latter package.

This commit fixes the issue by only copying relevant source files when cloning
a bundle's content into __ivy_ngcc__.

Fixes angular#29960
@JoostK JoostK added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer severity3: broken target: major This PR is targeted for the next major release risk: low labels Apr 21, 2019
@JoostK JoostK requested a review from a team as a code owner April 21, 2019 22:59
@ngbot ngbot bot added this to the needsTriage milestone Apr 21, 2019
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Thanks @JoostK - this looks like the right solution.

The trick is identifying what files are actually part of the entry-point program and which are outside.
I think that it is a fair assumption that .js files are part of the source and .d.ts files are not.
Certainly the latter statement is true (i.e. .d.ts files are always external from the point of view of ngcc).

But I wonder if there are occasions when there is a referenced .js file that should actually be regarded as outside the entry-point?
This is relevant given the current solution to #30017, which
is perferring .js files over .d.ts if they are next to each other. I think that it might be that in such a case we would end up including those .js files even though the reference to that module might be from a different entry-point.

What do you think @JoostK ?

In #29643 there is a new ModuleResolver, which
tries to do a better job than the TS and NodeJS module resolvers for the case of ngcc.
There is also a new NgccCompilerHost, which could be modified to use this new ModuleResolver.
Perhaps this would be able to cope better with that situation. In any case, any kind of fixes for module resolution should be added there and implemented into the NgccCompilerHost.

In the meantime, this PR looks good and we should land it - even though it will cause #29643 to need a rebase.

@JoostK
Copy link
Member Author

JoostK commented Apr 22, 2019

@petebacondarwin thanks for your extensive comment.

I share your concern regarding the trick of identifying when the source file is desired. The current logic in #30017 is what I think is sufficient: it treats secondary entry-points and other library imports correctly AFAIK. I am not sure how a .js file would creep in that should be regarded as outside of the package. If it does happen, it does not seem wrong to me that its relative imports are also resolved to the .js files. What I am trying to say is that in that case, the ts.Program is the ground truth of specifying what are sources, not the concept of an entry-point.

A custom module resolver as used in #29643 seems appealing to me though, if we can get it configured the way we want. Given that we have a testcase for the desired behaviour now, we can experiment a bit with other approaches.

I don't think #29643 would be too affected by this PR, it will however be somewhat affected by #30017. This PR in particular is necessary for the CLI to update to latest Angular packages as their integration tests are failing due to #29960 so we should land this ASAP.

@petebacondarwin petebacondarwin 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 22, 2019
@benlesh benlesh closed this in 8cba4e1 Apr 22, 2019
@JoostK JoostK deleted the ngcc-clobering branch April 22, 2019 16:21
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…ular#30020)

Previously, all of a program's files would be copied into the __ivy_ngcc__
folder where ngcc then writes its modifications into. The set of source files
in a program however is much larger than the source files contained within
the entry-point of interest, so many more files were copied than necessary.
Even worse, it may occur that an unrelated file in the program would collide
with an already existing source file, resulting in incorrectly overwriting
a file with unrelated content. This behavior has actually been observed
with @angular/animations and @angular/platform-browser/animations, where
the former package would overwrite declaration files of the latter package.

This commit fixes the issue by only copying relevant source files when cloning
a bundle's content into __ivy_ngcc__.

Fixes angular#29960

PR Close angular#30020
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes risk: low target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BrowserAnimationsModule ivy compile error
3 participants