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.

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

fix(ngcc): ensure that "inline exports" can be interpreted correctly #39267

Conversation

petebacondarwin
Copy link
Member

Previously, inline exports of the form exports.foo = <implementation>; were
being interpreted (by the ngtsc PartialInterpeter) as Reference objects.
This is not what is desired since it prevents the value of the export
from being unpacked, such as when analyzing NgModule declarations:

exports.directives = [Directive1, Directive2];

@NgImport({declarations: [exports.directives]})
class AppModule {}

In this example the interpreter would think that exports.directives
was a reference rather than an array that needs to be unpacked.

This bug was picked up by the ngcc-validation repository. See
angular/ngcc-validation#1990 and
https://circleci.com/gh/angular/ngcc-validation/17130

…s differently

Some inline declarations are of the form:

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

In this case the declaration `node` is `exports.<name>`.
When interpreting such inline declarations we actually want
to visit the `implementation` expression rather than visiting
the declaration `node`.

This commit adds `implementation?: ts.Expression` to the
`InlineDeclaration` type and updates the interpreter to visit
these expressions as described above.
Previously, inline exports of the form `exports.foo = <implementation>;` were
being interpreted (by the ngtsc `PartialInterpeter`) as `Reference` objects.
This is not what is desired since it prevents the value of the export
from being unpacked, such as when analyzing `NgModule` declarations:

```
exports.directives = [Directive1, Directive2];

@NgImport({declarations: [exports.directives]})
class AppModule {}
```

In this example the interpreter would think that `exports.directives`
was a reference rather than an array that needs to be unpacked.

This bug was picked up by the ngcc-validation repository. See
angular/ngcc-validation#1990 and
https://circleci.com/gh/angular/ngcc-validation/17130
@petebacondarwin petebacondarwin added type: bug/fix comp: ngcc target: minor This PR is targeted for the next minor release P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Oct 14, 2020
@ngbot ngbot bot added this to the needsTriage milestone Oct 14, 2020
@google-cla google-cla bot added the cla: yes label Oct 14, 2020
@petebacondarwin
Copy link
Member Author

I've targeted this PR at minor because the code that it sits on was only merged into master.
I'll need to create a new PR to fix the patch version.

@petebacondarwin petebacondarwin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 14, 2020
@gkalpak gkalpak changed the title @petebacondarwin fix(ngcc): ensure that "inline exports" can be interpreted correctly fix(ngcc): ensure that "inline exports" can be interpreted correctly Oct 14, 2020
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 14, 2020
@atscott atscott closed this in ac0016c Oct 14, 2020
atscott pushed a commit that referenced this pull request Oct 14, 2020
…39267)

Previously, inline exports of the form `exports.foo = <implementation>;` were
being interpreted (by the ngtsc `PartialInterpeter`) as `Reference` objects.
This is not what is desired since it prevents the value of the export
from being unpacked, such as when analyzing `NgModule` declarations:

```
exports.directives = [Directive1, Directive2];

@NgImport({declarations: [exports.directives]})
class AppModule {}
```

In this example the interpreter would think that `exports.directives`
was a reference rather than an array that needs to be unpacked.

This bug was picked up by the ngcc-validation repository. See
angular/ngcc-validation#1990 and
https://circleci.com/gh/angular/ngcc-validation/17130

PR Close #39267
@petebacondarwin petebacondarwin deleted the ngcc-inline-exports-commonjs branch October 15, 2020 07:23
@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 Nov 15, 2020
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 action: presubmit The PR is in need of a google3 presubmit cla: yes P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants