Skip to content

fix(ngcc): support inline exports.<name> = <decl> style exports in UMD files#38959

Closed
petebacondarwin wants to merge 17 commits intoangular:masterfrom
petebacondarwin:ngcc-export-identifier
Closed

fix(ngcc): support inline exports.<name> = <decl> style exports in UMD files#38959
petebacondarwin wants to merge 17 commits intoangular:masterfrom
petebacondarwin:ngcc-export-identifier

Conversation

@petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Sep 23, 2020

UMD files export values by assigning them to the exports variable.
When evaluating expressions ngcc was failing to cope with inline declarations
like exports.MyComponent = <decl>.

Fixes #38947

@petebacondarwin petebacondarwin added type: bug/fix 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 Sep 23, 2020
@ngbot ngbot bot modified the milestone: needsTriage Sep 23, 2020
@petebacondarwin petebacondarwin force-pushed the ngcc-export-identifier branch 2 times, most recently from 5c54067 to 114a530 Compare September 23, 2020 21:03
@petebacondarwin petebacondarwin marked this pull request as draft September 26, 2020 14:01
@petebacondarwin petebacondarwin added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 26, 2020
@petebacondarwin petebacondarwin removed the request for review from gkalpak September 26, 2020 14:01
@petebacondarwin petebacondarwin force-pushed the ngcc-export-identifier branch 6 times, most recently from 8fde932 to a5e8915 Compare September 29, 2020 21:22
@petebacondarwin petebacondarwin force-pushed the ngcc-export-identifier branch 2 times, most recently from 83ccf8f to 8a4280b Compare September 30, 2020 20:52
@petebacondarwin petebacondarwin changed the title fix(ngcc): map exports to the current module in UMD files fix(ngcc): support inline exports.<name> = <decl> style exports in UMD files Oct 1, 2020
@atscott
Copy link
Contributor

atscott commented Oct 9, 2020

Note: removing the merge label as this was found to break an internal build (pinged @petebacondarwin directly with details)

@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Oct 12, 2020
@ngbot
Copy link

ngbot bot commented Oct 12, 2020

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@atscott atscott added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Oct 12, 2020
@atscott atscott closed this in cee393d Oct 12, 2020
atscott pushed a commit that referenced this pull request Oct 12, 2020
…onReference()` (#38959)

There is no need to check that the `ref.node` is of any particular type
because immediately after this check the entry is tested to see if it passes
`isClassDeclarationReference()`.

The only difference is that the error that is reported is slightly different
in the case that it is a `ref` but not one of the TS node types.

Previously:

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                className} is not a reference`
```

now

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                  className} is not a class`
```

Arguably the previous message was wrong, since this entry IS a reference
but is not a class.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
The `isAssignment` and `isAssignmentStatement` are not used in this file.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
This clarifies that this is specifically about statements of the form
`exports.<name> = <declaration>`, rather than a general export
statement such as `export class <ClassName> { ... }`.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
…38959)

If the `symbol` for the given `node` does not exist then there is no
point in creating the `map`.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
The `SIMPLE_CLASS_FILE` contained a `ChildClass` that had an
internal aliases implementation and extended a `SuperClass` base
class. The call to `__extends` was using the wrong argument for
the child class.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
UMD files export values by assigning them to an `exports` variable.
When evaluating expressions ngcc was failing to cope with expressions
like `exports.MyComponent`.

This commit fixes the `UmdReflectionHost.getDeclarationOfIdentifier()`
method to map the `exports` variable to the current source file.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
Sometimes UMD exports appear in the following form:

```
exports.MyClass = alias1 = alias2 = <<declaration>>
```

Previously the declaration of the export would have been captured
as `alias1 = alias2 = <<declaration>>`, which the `PartialInterpreter`
would have failed on, since it cannot handle assignments.

Now we skip over these aliases capturing only the `<<declaration>>`
expression.

Fixes #38947

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
…aration()` (#38959)

The new function does not try to restrict the kind of AST node that it
finds, leaving that to the caller. This will make it more resuable in the
UMD reflection host.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
…38959)

The protected helper functions can then be overridden by subclasses of the
Esm2015ReflectionHost.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
…t to code format (#38959)

Previously `getDeclaration()` would only return the first node that matched
the name passed in and then assert the predicate on this single node.
It also only considered a subset of possible declaration types that we might
care about.

Now the function will parse the whole tree collecting an array of all the
nodes that match the name. It then filters this array based on the predicate
and only errors if the filtered array is empty.

This makes this function much more resilient to more esoteric code formats
such as UMD.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
This makes these tests more resilient to changes in the test code
structure. For example switching from

```
var SomeClass = <implementation>;
exports.SomeClass = SomeClass;
```

to

```
exports.SomeClass = <implementation>;
```

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
Previously the `ConcreteDeclaration` and `InlineDeclaration` had
different properties for the underlying node type. And the `InlineDeclaration`
did not store a value that represented its declaration.

It turns out that a natural declaration node for an inline type is the
expression. For example in UMD/CommonJS this would be the `exports.<name>`
property access node.

So this expression is now used for the `node` of `InlineDeclaration` types
and the `expression` property is dropped.

To support this the codebase has been refactored to use a new `DeclarationNode`
type which is a union of `ts.Declaration|ts.Expression` instead of `ts.Declaration`
throughout.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
Previously these tests were checking multiple specific expression
types. The new helper function is more general and will also support
`PropertyAccessExpression` nodes for `InlineDeclaration` types.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
Previously, any declarations that were defined "inline" were not
recognised by the `UmdReflectionHost`.

For example, the following syntax was completely unrecognized:

```ts
var Foo_1;
exports.Foo = Foo_1 = (function() {
  function Foo() {}
  return Foo;
})();
exports.Foo = Foo_1 = __decorate(SomeDecorator, Foo);
```

Such inline classes were ignored and not processed by ngcc.

This lack of processing led to failures in Ivy applications that relied
on UMD formats of libraries such as `syncfusion/ej2-angular-ui-components`.

Now all known inline UMD exports are recognized and processed accordingly.

Fixes #38947

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
Previously directive "queries" that relied upon a namespaced type

```ts
queries: {
  'mcontent': new core.ContentChild('test2'),
}
```

caused an error to be thrown. This is now supported.

PR Close #38959
atscott pushed a commit that referenced this pull request Oct 12, 2020
…identity (#38959)

Previously the `node.name` property was only checked to ensure it was
defined. But that meant that it was a `ts.BindingName`, which also includes
`ts.BindingPattern`, which we do not support. But these helper methods were
forcefully casting the value to `ts.Identifier.

Now we also check that the `node.name` is actually an `ts.Identifier`.

PR Close #38959
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 14, 2020
…lar#38959)

The message now also reports the name of the predicate function
that failed.

PR Close angular#38959
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 14, 2020
…onReference()` (angular#38959)

There is no need to check that the `ref.node` is of any particular type
because immediately after this check the entry is tested to see if it passes
`isClassDeclarationReference()`.

The only difference is that the error that is reported is slightly different
in the case that it is a `ref` but not one of the TS node types.

Previously:

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                className} is not a reference`
```

now

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                  className} is not a class`
```

Arguably the previous message was wrong, since this entry IS a reference
but is not a class.

PR Close angular#38959
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 14, 2020
The `isAssignment` and `isAssignmentStatement` are not used in this file.

PR Close angular#38959
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 14, 2020
…ar#38959)

This clarifies that this is specifically about statements of the form
`exports.<name> = <declaration>`, rather than a general export
statement such as `export class <ClassName> { ... }`.

PR Close angular#38959
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 14, 2020
…ngular#38959)

If the `symbol` for the given `node` does not exist then there is no
point in creating the `map`.

PR Close angular#38959
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 14, 2020
The `SIMPLE_CLASS_FILE` contained a `ChildClass` that had an
internal aliases implementation and extended a `SuperClass` base
class. The call to `__extends` was using the wrong argument for
the child class.

PR Close angular#38959
@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.

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

error NG1010: Value at position 0 in the NgModule.declarations of Module is not a reference

7 participants