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.

Sign up for GitHub

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

Jump to bottom

Fix g3 core imports #27998

Closed
wants to merge 2 commits into from
Closed

Fix g3 core imports #27998

wants to merge 2 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jan 8, 2019

Based on #27743 (for now)

@alxhub alxhub requested review from a team as code owners January 8, 2019 21:19
@alxhub alxhub requested a review from vikerman January 8, 2019 21:23
@mary-poppins
Copy link

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 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
Copy link

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

@mary-poppins
Copy link

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

@JoostK
Copy link
Member

JoostK commented Jan 9, 2019

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 requested a review from a team as a code owner January 9, 2019 16:35
@alxhub
Copy link
Member Author

alxhub commented Jan 9, 2019

@JoostK nice catch, done!

@mary-poppins
Copy link

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
Copy link

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

@mary-poppins
Copy link

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

petebacondarwin
petebacondarwin approved these changes Jan 9, 2019
View reviewed changes
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.

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

packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_rewriter.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngcc/src/rendering/renderer.ts Outdated Show resolved Hide resolved
@mary-poppins
Copy link

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

gkalpak
gkalpak reviewed Jan 10, 2019
View reviewed changes
packages/compiler-cli/src/ngtsc/transform/src/transform.ts Outdated
const constantPool = new ConstantPool();
const importManager = new ImportManager(coreImportsFrom !== null);
const importManager = new ImportManager(importRewriter || undefined);
Copy link
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?

Copy link
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.

packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts Outdated
if (ts.isImportDeclaration(stmt) && ts.isStringLiteral(stmt.moduleSpecifier) &&
stmt.moduleSpecifier.text === '@angular/core') {
const rewrittenModuleSpecifier =
importRewriter.rewriteSpecifier('@angular/core', sourceFilePath);
// Update the import path to point to the correct file (coreImportsFrom).
Copy link
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_rewriter.ts
return true;
}
}
rewriteSymbol(symbol: string, specifier: string): string {
Copy link
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 😞

Copy link
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
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
@mary-poppins
Copy link

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
Copy link
Member Author

alxhub commented Jan 10, 2019

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

@AndrewKushnir
Copy link
Contributor

Presubmit

AndrewKushnir
AndrewKushnir approved these changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@alxhub @wKoza
refactor(ivy): extract import rewriting into a separate interface (an…
5dbc234 
…gular#27998)

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.

PR Close angular#27998
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@alxhub @wKoza
fix(ivy): properly rewrite imports in generated factory shims (angula…
ce75d7a 
…r#27998)

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 angular#27998
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@alxhub @wKoza
refactor(ivy): extract import rewriting into a separate interface (an…
6567f99 
…gular#27998)

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.

PR Close angular#27998
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@alxhub @wKoza
fix(ivy): properly rewrite imports in generated factory shims (angula…
c514fd4 
…r#27998)

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 angular#27998
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@alxhub @ngfelixl
refactor(ivy): extract import rewriting into a separate interface (an…
79bfb8b 
…gular#27998)

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.

PR Close angular#27998
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@alxhub @ngfelixl
fix(ivy): properly rewrite imports in generated factory shims (angula…
d5e1ca2 
…r#27998)

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 angular#27998
@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.
Reviewers

@gkalpak gkalpak gkalpak left review comments

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@vikerman vikerman vikerman approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

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.

None yet
9 participants
@alxhub @mary-poppins @JoostK @AndrewKushnir @petebacondarwin @vikerman @gkalpak @kara @googlebot