Skip to content

Fix g3 core imports - #27998

Closed
alxhub wants to merge 2 commits into
angular:masterfrom
alxhub:fix-g3-core-imports
Closed

Fix g3 core imports#27998
alxhub wants to merge 2 commits into
angular:masterfrom
alxhub:fix-g3-core-imports

Conversation

@alxhub

@alxhub alxhub commented Jan 8, 2019

Copy link
Copy Markdown
Member

Based on #27743 (for now)

@alxhub
alxhub requested review from a team January 8, 2019 21:19
@alxhub
alxhub requested a review from vikerman January 8, 2019 21:23
@mary-poppins

mary-poppins commented Jan 8, 2019

Copy link
Copy Markdown

You can preview 0f8b677 at https://pr27998-0f8b677.ngbuilds.io/.

@kara kara added the comp: ivy label Jan 8, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Jan 8, 2019
@alxhub
alxhub force-pushed the fix-g3-core-imports branch from 0f8b677 to 317e2e4 Compare January 9, 2019 00:41
@alxhub
alxhub requested a review from petebacondarwin January 9, 2019 00:42
@alxhub
alxhub force-pushed the fix-g3-core-imports branch from 317e2e4 to 525cdfb Compare January 9, 2019 00:43
@alxhub alxhub added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Jan 9, 2019
@mary-poppins

mary-poppins commented Jan 9, 2019

Copy link
Copy Markdown

You can preview 317e2e4 at https://pr27998-317e2e4.ngbuilds.io/.

@mary-poppins

mary-poppins commented Jan 9, 2019

Copy link
Copy Markdown

You can preview 525cdfb at https://pr27998-525cdfb.ngbuilds.io/.

@JoostK

JoostK commented Jan 9, 2019

Copy link
Copy Markdown
Member

Nice work! I can't properly review from my phone (on holiday this week) but I have one request: could you update the comment in r3_symbols.ts to point to the new file where imports are rewritten? I put a reference to translator.ts there which is now invalidated.

@alxhub
alxhub force-pushed the fix-g3-core-imports branch from 525cdfb to 15eaf33 Compare January 9, 2019 16:35
@alxhub
alxhub requested a review from a team January 9, 2019 16:35
@alxhub

alxhub commented Jan 9, 2019

Copy link
Copy Markdown
Member Author

@JoostK nice catch, done!

@mary-poppins

mary-poppins commented Jan 9, 2019

Copy link
Copy Markdown

You can preview 15eaf33 at https://pr27998-15eaf33.ngbuilds.io/.

@alxhub
alxhub force-pushed the fix-g3-core-imports branch 2 times, most recently from c3c77a5 to e2b7501 Compare January 9, 2019 18:35
@mary-poppins

mary-poppins commented Jan 9, 2019

Copy link
Copy Markdown

You can preview c3c77a5 at https://pr27998-c3c77a5.ngbuilds.io/.

@mary-poppins

mary-poppins commented Jan 9, 2019

Copy link
Copy Markdown

You can preview e2b7501 at https://pr27998-e2b7501.ngbuilds.io/.

petebacondarwin
petebacondarwin approved these changes Jan 9, 2019
View reviewed changes

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From an ngcc point-of-view this looks like a really nice code tidy-up. A couple of minor nits.

Comment thread packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_rewriter.ts Resolved
Comment thread packages/compiler-cli/src/ngcc/src/rendering/renderer.ts Resolved
@alxhub
alxhub force-pushed the fix-g3-core-imports branch from e2b7501 to 3fc9187 Compare January 9, 2019 19:28
@mary-poppins

mary-poppins commented Jan 9, 2019

Copy link
Copy Markdown

You can preview 3fc9187 at https://pr27998-3fc9187.ngbuilds.io/.

gkalpak
gkalpak reviewed Jan 10, 2019
View reviewed changes
Comment thread packages/compiler-cli/src/ngtsc/transform/src/transform.ts Outdated

@gkalpak gkalpak Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't importRewriter always defined?

@alxhub alxhub Jan 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, fixed.

Comment thread packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts Outdated

@gkalpak gkalpak Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

coreImportsFrom does no longer exist.

@alxhub alxhub Jan 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_rewriter.ts Outdated

@gkalpak gkalpak Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No empty line between methods 😞

@alxhub alxhub Jan 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed.

@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 10, 2019
@kara kara removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 10, 2019
alxhub added 2 commits January 10, 2019 09:15
@alxhub
refactor(ivy): extract import rewriting into a separate interface
18f3e23 
Currently the ImportManager class handles various rewriting actions of
imports when compiling @angular/core. This is required as code compiled
within @angular/core cannot import from '@angular/core'. To work around
this, imports are rewritten to get core symbols from a particular file,
r3_symbols.ts.

In this refactoring, this rewriting logic is moved out of the ImportManager
and put behind an interface, ImportRewriter. There are three implementers
of the interface:

* NoopImportRewriter, used for compiling all non-core packages.
* R3SymbolsImportRewriter, used when ngtsc compiles @angular/core.
* NgccFlatImportRewriter, used when ngcc compiles @angular/core (special
  logic is needed because ngcc has to rewrite imports in flat bundles
  differently than in non-flat bundles).

This is a precursor to using this rewriting logic in other contexts besides
the ImportManager.
@alxhub
fix(ivy): properly rewrite imports in generated factory shims
cfb588d 
Generated factory shims can import from @angular/core. However, we have
special logic in place to rewrite self-imports when generating code for
@angular/core.

This commit leverages the new standalone ImportRewriter interface to
properly rewrite imports in generated factory shims. Before this fix,
a generated factory file for core would look like:

```typescript
import * as i0 from './r3_symbols';

export var ApplicationModuleNgFactory = new ɵNgModuleFactory(...);
```

This is invalid, as ɵNgModuleFactory is just NgModuleFactory when imported
via r3_symbols.

FW-881 #resolve
@alxhub
alxhub force-pushed the fix-g3-core-imports branch from 3fc9187 to cfb588d Compare January 10, 2019 17:17
@mary-poppins

mary-poppins commented Jan 10, 2019

Copy link
Copy Markdown

You can preview cfb588d at https://pr27998-cfb588d.ngbuilds.io/.

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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 Jan 10, 2019
@alxhub

alxhub commented Jan 10, 2019

Copy link
Copy Markdown
Member Author

fw-forms and fw-dev-infra are not needed.

@alxhub
alxhub requested a review from AndrewKushnir January 10, 2019 17:46
@AndrewKushnir

AndrewKushnir commented Jan 10, 2019

Copy link
Copy Markdown
Contributor

Presubmit

AndrewKushnir
AndrewKushnir approved these changes Jan 10, 2019
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, one nit is the way we work with '@angular/core': in some places we put it in a const and reference it, in other places, we just use this as a string to compare with a given value. We should probably unify that at some point (I noticed that in other places as well). This is a minor thing and definitely not a blocker to approve this PR :)

AndrewKushnir pushed a commit that referenced this pull request Jan 10, 2019
@alxhub @AndrewKushnir
fix(ivy): properly rewrite imports in generated factory shims (#27998)
6003145 
Generated factory shims can import from @angular/core. However, we have
special logic in place to rewrite self-imports when generating code for
@angular/core.

This commit leverages the new standalone ImportRewriter interface to
properly rewrite imports in generated factory shims. Before this fix,
a generated factory file for core would look like:

```typescript
import * as i0 from './r3_symbols';

export var ApplicationModuleNgFactory = new ɵNgModuleFactory(...);
```

This is invalid, as ɵNgModuleFactory is just NgModuleFactory when imported
via r3_symbols.

FW-881 #resolve

PR Close #27998
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link
Copy Markdown

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.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes
+3 more reviewers
@gkalpak gkalpak gkalpak left review comments
@petebacondarwin petebacondarwin petebacondarwin approved these changes
@vikerman vikerman vikerman approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@alxhub alxhub

Labels

action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

9 participants

@alxhub @mary-poppins @JoostK @AndrewKushnir @petebacondarwin @vikerman @gkalpak @kara @googlebot