fix(ivy): prevent ngcc from referencing missing ɵsetClassMetadata by JoostK · Pull Request #27055 · angular/angular

JoostK · 2018-11-11T18:19:47Z

When ngtsc compiles @angular/core, it rewrites core imports to the
r3_symbols.ts file that exposes all internal symbols under their
external name. When creating the FESM bundle, the r3_symbols.ts file
causes the external symbol names to be rewritten to their internal name.

Under ngcc compilations of FESM bundles, the indirection of
r3_symbols.ts is no longer in place such that the external names are
retained in the bundle. Previously, the external name ɵdefineNgModule
was explicitly declared internally to resolve this issue, but the
recently added setClassMetadata was not declared as such, causing
runtime errors.

Instead of relying on the r3_symbols.ts file to perform the rewrite of
the external modules to their internal variants, the translation is
moved into the ImportManager during the compilation itself. This
avoids the need for providing the external name manually.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

ngcc compiled @angular/core would reference setClassMetadata by its private name ɵsetClassMetadata which wouldn't be available during runtime.

What is the new behavior?

External references are rewritten to their internal names during compilation, instead of relying on r3_symbols.ts as that is not in use when ngcc processes the FESM bundles of @angular/core.

Does this PR introduce a breaking change?

Yes
No

Other information

petebacondarwin · 2018-11-11T18:59:24Z

The source needs formatting: https://circleci.com/gh/angular/angular/104308

petebacondarwin · 2018-11-11T19:00:43Z

It seems that these changes are still not enough to fix the FESM5 situation. The ngcc-CLI integration test is complaining: https://circleci.com/gh/angular/angular/104316.

JoostK · 2018-11-11T21:57:13Z

I figured out why the integration test fails:

The .d.ts transform is preferred to run for the FESM2015 bundle, for which no r3_symbols file can be located. Hence, the imports that are generated for @angular/core are not rewritten at all. Because the symbols are now rewritten to their internal names, they fail to be resolved from @angular/core (as expected).

If I force the .d.ts transform to run for ESM2015 bundles, I do end up with a relative import but unfortunately that is of the form:

import * as ɵngcc0 from '../esm2015/src/r3_symbols';

Makes sense, given that the ESM2015 bundle path was used for searching for r3_symbols. For the .d.ts transform we should find the r3_symbols.d.ts file instead.

Please mark this PR as WIP for now, I will try to resolve the above issue tomorrow evening.

JoostK · 2018-11-11T23:30:12Z

Alright, I pushed a very dirty implementation to see if it would work, and it does. CircleCI isn't being triggered for some reason but I have seen the integration tests pass.

The seconds commit needs some cleanup as it has currently gone wild. Also, tests are needed to verify that this works as expected.

alfaproject · 2018-11-12T19:58:02Z

Should this be exported from a utility class? Duplicating it already proved brittle

petebacondarwin · 2018-11-13T09:31:56Z

I like the concept of an object to hold this stuff, I am not happy with the name though. Bundle is too generic.

Also I still switch back and forth between passing everything into the constructor versus passing everything to the renderProgram method versus (our current situation) of passing some things to each.

When ngtsc compiles @angular/core, it rewrites core imports to the r3_symbols.ts file that exposes all internal symbols under their external name. When creating the FESM bundle, the r3_symbols.ts file causes the external symbol names to be rewritten to their internal name. Under ngcc compilations of FESM bundles, the indirection of r3_symbols.ts is no longer in place such that the external names are retained in the bundle. Previously, the external name `ɵdefineNgModule` was explicitly declared internally to resolve this issue, but the recently added `setClassMetadata` was not declared as such, causing runtime errors. Instead of relying on the r3_symbols.ts file to perform the rewrite of the external modules to their internal variants, the translation is moved into the `ImportManager` during the compilation itself. This avoids the need for providing the external name manually.

…ve imports

JoostK · 2018-11-20T20:11:03Z

@mhevery FYI, I'm planning on rebasing this on top of #26906 as it's quite conflicty, and @petebacondarwin's PR should be considered ahead of this one.

petebacondarwin · 2018-11-21T07:37:56Z

And after a review from Alex R, I am currently rewriting some portion of that PR, but it should be ready in a few hours.

alxhub · 2018-11-20T20:54:03Z

    const isCore = entryPoint.name === '@angular/core';
-    const r3SymbolsPath = isCore ? this.findR3SymbolsPath(dirname(entryPointFilePath)) : null;
+    const r3SymbolsPath =
+        isCore ? this.findR3SymbolsPath(dirname(entryPointFilePath), 'r3_symbols.js') : null;


Does this work even if we're compiling a FESM entrypoint?

Yes. For FESMs, the NgccImportManager will prevent core imports from being generated if core itself is being compiled.

angular/packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_manager.ts

Lines 17 to 19 in 789d323

if (this.isFlat && this.isCore && moduleName === '@angular/core') {

return {moduleImport: null, symbol: this.rewriteSymbol(moduleName, symbol)};

}

Glad that is sorted :-)

petebacondarwin · 2018-11-21T14:03:40Z

@mhevery - can you review this for integration and core PullApprove?

mhevery · 2018-11-21T16:20:20Z

http://test/OCL:222410910:BASE:222410955:1542817212252:aab09c58

…rts (#27055) PR Close #27055

angular-automatic-lock-bot · 2019-09-14T10:21:56Z

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

googlebot added the cla: yes label Nov 11, 2018

petebacondarwin reviewed Nov 11, 2018

View reviewed changes

Comment thread packages/core/src/core_render3_private_export.ts Outdated

petebacondarwin requested review from alxhub and mhevery November 11, 2018 19:01

petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours freq2: medium severity3: broken target: major This PR is targeted for the next major release labels Nov 11, 2018

ngbot Bot modified the milestone: needsTriage Nov 11, 2018

petebacondarwin modified the milestones: needsTriage, Ivy Nov 11, 2018

JoostK force-pushed the ngcc-fix-invalid-setmetadata-call branch 4 times, most recently from 40dfdab to 50c4f02 Compare November 11, 2018 23:19

petebacondarwin added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 12, 2018

gkalpak reviewed Nov 12, 2018

View reviewed changes

Comment thread packages/compiler-cli/src/ngtsc/util/src/path.ts Outdated

JoostK force-pushed the ngcc-fix-invalid-setmetadata-call branch from 37a0a43 to a1f1271 Compare November 12, 2018 19:14

alfaproject reviewed Nov 12, 2018

View reviewed changes

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Nov 12, 2018

petebacondarwin assigned alxhub and petebacondarwin Nov 12, 2018

petebacondarwin reviewed Nov 13, 2018

View reviewed changes

JoostK added 4 commits November 16, 2018 22:02

fix(ivy): let ngcc transform @angular/core typings with relative imports

dcbead9

fixup! fix(ivy): prevent ngcc from referencing missing ɵsetClassMetadata

9811ee4

fixup! fix(ivy): let ngcc transform @angular/core typings with relati…

789d323

…ve imports

JoostK force-pushed the ngcc-fix-invalid-setmetadata-call branch from a1f1271 to 789d323 Compare November 18, 2018 20:18

mhevery self-assigned this Nov 20, 2018

alxhub approved these changes Nov 21, 2018

View reviewed changes

JoostK mentioned this pull request Nov 21, 2018

ngcc: re-export private NgModule declarations #26906

Closed

14 tasks

mhevery approved these changes Nov 21, 2018

View reviewed changes

mhevery added the action: merge The PR is ready for merge by the caretaker label Nov 21, 2018

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 21, 2018

mhevery closed this in c8c8648 Nov 21, 2018

mhevery pushed a commit that referenced this pull request Nov 21, 2018

fix(ivy): let ngcc transform @angular/core typings with relative impo…

0d9b27f

…rts (#27055) PR Close #27055

JoostK deleted the ngcc-fix-invalid-setmetadata-call branch December 10, 2018 22:00

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

	if (this.isFlat && this.isCore && moduleName === '@angular/core') {
	return {moduleImport: null, symbol: this.rewriteSymbol(moduleName, symbol)};
	}

Conversation

JoostK commented Nov 11, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Uh oh!

petebacondarwin commented Nov 11, 2018

Uh oh!

petebacondarwin commented Nov 11, 2018

Uh oh!

JoostK commented Nov 11, 2018

Uh oh!

JoostK commented Nov 11, 2018

Uh oh!

Uh oh!

alfaproject Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

JoostK commented Nov 20, 2018

Uh oh!

petebacondarwin commented Nov 21, 2018

Uh oh!

alxhub Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

JoostK Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Nov 21, 2018

Uh oh!

mhevery commented Nov 21, 2018

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants