fix(ngcc): capture UMD/CommonJS inner class implementation nodes correctly #39346

petebacondarwin · 2020-10-20T18:39:02Z

Previously, UMD/CommonJS class inline declarations of the form:

exports.Foo = (function() { function Foo(); return Foo; })();

were capturing the whole IIFE as the implementation, rather than
the inner class (i.e. function Foo() {} in this case). This caused
the interpreter to break when it was trying to access such an export,
since it would try to evaluate the IIFE rather than treating it as a class
declaration.

gkalpak

LGTM with a couple of comments:

It would be cool if we could have an integration test showing all the bits working together (unless it would be too messy).
Commit message typo: Fool --> Foo 😁

gkalpak · 2020-10-21T12:42:08Z

packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts

@@ -342,7 +342,8 @@ export class StaticInterpreter {
  }

  private visitAmbiguousDeclaration(decl: Declaration, declContext: Context) {
-    return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined ?
+    return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined &&
+            !isDeclaration(decl.implementation) ?
        // Inline declarations with an `implementation` should be visited as expressions


This comment could be updated to mention the new requirement for implementation to be a ts.Expression.

gkalpak · 2020-10-21T12:45:18Z

packages/compiler-cli/ngcc/test/host/umd_host_spec.ts

-    return SomeClass;
-  }())`,
+                  'SomeClass',
+                  `SomeClass = (function() {\n    function SomeClass() {}\n    return SomeClass;\n  }())`,


Suggested change

`SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())`,

'SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())',

petebacondarwin · 2020-10-21T16:09:35Z

Hmm, an integration test... Let me look into that. Trouble is that we don't have any UMD packages to test against.

alxhub · 2020-10-21T16:31:15Z

Presubmit

petebacondarwin · 2020-10-21T17:04:10Z

@gkalpak - I managed to create an integration test that actually fails without this fix!!

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts

…ctly Previously, UMD/CommonJS class inline declarations of the form: ```ts exports.Foo = (function() { function Foo(); return Foo; })(); ``` were capturing the whole IIFE as the implementation, rather than the inner class (i.e. `function Foo() {}` in this case). This caused the interpreter to break when it was trying to access such an export, since it would try to evaluate the IIFE rather than treating it as a class declaration.

…e correctly

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts

…e correctly

The compiler uses a `Reference` abstraction to refer to TS nodes that it needs to refer to from other parts of the source. Such references keep track of any identifiers that represent the referenced node. Prior to this commit, the compiler (and specifically `ReferenceEmitter` classes) assumed that the reference identifiers are always free standing. In other words a reference identifier would be an expression like `FooDirective` in the expression `class FooDirective {}`. But in UMD/CommonJS source, a reference can actually refer to an "exports" declaration of the form `exports.FooDirective = ...`. In such cases the `FooDirective` identifier is not free-standing since it is part of a property access, so the `ReferenceEmitter` should take this into account when emitting an expression that refers to such a `Reference`. This commit changes the `LocalIdentifierStrategy` reference emitter so that if the `node` being referenced is not a declaration itself and is in the current file, then it should be used directly, rather than trying to use one of its identifiers.

AndrewKushnir · 2020-10-23T16:34:00Z

Presubmit.

AndrewKushnir · 2020-10-23T19:02:16Z

FYI, presubmit is successful (I've updated g3 status for this PR).

petebacondarwin · 2020-10-23T19:03:08Z

Thanks @AndrewKushnir

…ctly (#39346) Previously, UMD/CommonJS class inline declarations of the form: ```ts exports.Foo = (function() { function Foo(); return Foo; })(); ``` were capturing the whole IIFE as the implementation, rather than the inner class (i.e. `function Foo() {}` in this case). This caused the interpreter to break when it was trying to access such an export, since it would try to evaluate the IIFE rather than treating it as a class declaration. PR Close #39346

The compiler uses a `Reference` abstraction to refer to TS nodes that it needs to refer to from other parts of the source. Such references keep track of any identifiers that represent the referenced node. Prior to this commit, the compiler (and specifically `ReferenceEmitter` classes) assumed that the reference identifiers are always free standing. In other words a reference identifier would be an expression like `FooDirective` in the expression `class FooDirective {}`. But in UMD/CommonJS source, a reference can actually refer to an "exports" declaration of the form `exports.FooDirective = ...`. In such cases the `FooDirective` identifier is not free-standing since it is part of a property access, so the `ReferenceEmitter` should take this into account when emitting an expression that refers to such a `Reference`. This commit changes the `LocalIdentifierStrategy` reference emitter so that if the `node` being referenced is not a declaration itself and is in the current file, then it should be used directly, rather than trying to use one of its identifiers. PR Close #39346

…ctly (#39346) Previously, UMD/CommonJS class inline declarations of the form: ```ts exports.Foo = (function() { function Foo(); return Foo; })(); ``` were capturing the whole IIFE as the implementation, rather than the inner class (i.e. `function Foo() {}` in this case). This caused the interpreter to break when it was trying to access such an export, since it would try to evaluate the IIFE rather than treating it as a class declaration. PR Close #39346

The compiler uses a `Reference` abstraction to refer to TS nodes that it needs to refer to from other parts of the source. Such references keep track of any identifiers that represent the referenced node. Prior to this commit, the compiler (and specifically `ReferenceEmitter` classes) assumed that the reference identifiers are always free standing. In other words a reference identifier would be an expression like `FooDirective` in the expression `class FooDirective {}`. But in UMD/CommonJS source, a reference can actually refer to an "exports" declaration of the form `exports.FooDirective = ...`. In such cases the `FooDirective` identifier is not free-standing since it is part of a property access, so the `ReferenceEmitter` should take this into account when emitting an expression that refers to such a `Reference`. This commit changes the `LocalIdentifierStrategy` reference emitter so that if the `node` being referenced is not a declaration itself and is in the current file, then it should be used directly, rather than trying to use one of its identifiers. PR Close #39346

angular-automatic-lock-bot · 2020-11-24T16:29:45Z

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

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release comp: ngcc labels Oct 20, 2020

ngbot bot modified the milestone: needsTriage Oct 20, 2020

google-cla bot added the cla: yes label Oct 20, 2020

pullapprove bot requested a review from alxhub October 20, 2020 18:39

petebacondarwin force-pushed the ngcc-umd-inline-exports branch from d7848e3 to 5ca8a45 Compare October 20, 2020 18:40

petebacondarwin changed the title ~~fix(ngcc): capture UMD inner class implementation correctly~~ fix(ngcc): capture UMD/CommonJS inner class implementation correctly Oct 20, 2020

petebacondarwin changed the title ~~fix(ngcc): capture UMD/CommonJS inner class implementation correctly~~ fix(ngcc): capture UMD/CommonJS inner class implementation nodes correctly Oct 20, 2020

petebacondarwin requested review from gkalpak and JoostK October 20, 2020 18:40

gkalpak approved these changes Oct 21, 2020

View reviewed changes

gkalpak 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 Oct 21, 2020

petebacondarwin force-pushed the ngcc-umd-inline-exports branch from 5ca8a45 to a538f10 Compare October 21, 2020 16:09

JoostK reviewed Oct 21, 2020

View reviewed changes

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts Outdated Show resolved Hide resolved

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts Outdated Show resolved Hide resolved

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts Show resolved Hide resolved

JoostK 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 Oct 21, 2020

petebacondarwin added 4 commits October 21, 2020 21:11

fixup! fix(ngcc): capture UMD/CommonJS inner class implementation nod…

b5d4cef

…e correctly

fixup! fix(ngcc): capture UMD/CommonJS inner class implementation nod…

2adf4c1

…e correctly

fixup! fix(ngcc): capture UMD/CommonJS inner class implementation nod…

6c16cbd

…e correctly

petebacondarwin force-pushed the ngcc-umd-inline-exports branch from b88f13e to 6c16cbd Compare October 21, 2020 20:12

petebacondarwin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 21, 2020

petebacondarwin requested a review from JoostK October 21, 2020 21:19

JoostK requested changes Oct 21, 2020

View reviewed changes

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts Outdated Show resolved Hide resolved

fixup! fix(ngcc): capture UMD/CommonJS inner class implementation nod…

5f122c4

…e correctly

petebacondarwin force-pushed the ngcc-umd-inline-exports branch from 6152d10 to 2d158b3 Compare October 22, 2020 10:41

petebacondarwin force-pushed the ngcc-umd-inline-exports branch from 2d158b3 to 817c08a Compare October 22, 2020 11:07

petebacondarwin requested a review from JoostK October 22, 2020 12:33

petebacondarwin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 22, 2020

JoostK approved these changes Oct 22, 2020

View reviewed changes

petebacondarwin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 22, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Oct 23, 2020

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Oct 23, 2020

petebacondarwin removed the request for review from alxhub October 23, 2020 19:02

alxhub closed this in 413b552 Oct 23, 2020

petebacondarwin deleted the ngcc-umd-inline-exports branch October 24, 2020 09:40

gkalpak mentioned this pull request Oct 24, 2020

build(deps): update dependency igniteui-angular to v10.2.2 angular/ngcc-validation#1883

Merged

1 task

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngcc): capture UMD/CommonJS inner class implementation nodes correctly #39346

fix(ngcc): capture UMD/CommonJS inner class implementation nodes correctly #39346

petebacondarwin commented Oct 20, 2020 •

edited by gkalpak

gkalpak left a comment

gkalpak Oct 21, 2020 •

edited

gkalpak Oct 21, 2020

petebacondarwin commented Oct 21, 2020

alxhub commented Oct 21, 2020

petebacondarwin commented Oct 21, 2020

AndrewKushnir commented Oct 23, 2020

AndrewKushnir commented Oct 23, 2020

petebacondarwin commented Oct 23, 2020

angular-automatic-lock-bot bot commented Nov 24, 2020

	`SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())`,
	'SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())',

fix(ngcc): capture UMD/CommonJS inner class implementation nodes correctly #39346

fix(ngcc): capture UMD/CommonJS inner class implementation nodes correctly #39346

Conversation

petebacondarwin commented Oct 20, 2020 • edited by gkalpak

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Oct 21, 2020 • edited

Choose a reason for hiding this comment

gkalpak Oct 21, 2020

Choose a reason for hiding this comment

petebacondarwin commented Oct 21, 2020

alxhub commented Oct 21, 2020

petebacondarwin commented Oct 21, 2020

AndrewKushnir commented Oct 23, 2020

AndrewKushnir commented Oct 23, 2020

petebacondarwin commented Oct 23, 2020

angular-automatic-lock-bot bot commented Nov 24, 2020

petebacondarwin commented Oct 20, 2020 •

edited by gkalpak

gkalpak Oct 21, 2020 •

edited