Skip to content

ngcc: re-export private NgModule declarations#26906

Closed
petebacondarwin wants to merge 13 commits into
angular:masterfrom
petebacondarwin:ngcc-private-declarations
Closed

ngcc: re-export private NgModule declarations#26906
petebacondarwin wants to merge 13 commits into
angular:masterfrom
petebacondarwin:ngcc-private-declarations

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mary-poppins
Copy link
Copy Markdown

You can preview fd6cbc6 at https://pr26906-fd6cbc6.ngbuilds.io/.

@petebacondarwin petebacondarwin force-pushed the ngcc-private-declarations branch from fd6cbc6 to 1ae87b6 Compare November 1, 2018 22:07
@mary-poppins
Copy link
Copy Markdown

You can preview 1ae87b6 at https://pr26906-1ae87b6.ngbuilds.io/.

@petebacondarwin petebacondarwin force-pushed the ngcc-private-declarations branch from 1ae87b6 to 159b98e Compare November 1, 2018 22:13
Comment thread packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts Outdated
@petebacondarwin petebacondarwin force-pushed the ngcc-private-declarations branch from 159b98e to 86732c9 Compare November 12, 2018 13:28
@mary-poppins
Copy link
Copy Markdown

You can preview 86732c9 at https://pr26906-86732c9.ngbuilds.io/.

@petebacondarwin petebacondarwin force-pushed the ngcc-private-declarations branch from 86732c9 to 9ba0954 Compare November 12, 2018 13:59
@mary-poppins
Copy link
Copy Markdown

You can preview 9ba0954 at https://pr26906-9ba0954.ngbuilds.io/.

@petebacondarwin petebacondarwin force-pushed the ngcc-private-declarations branch from 9ba0954 to 16fa15f Compare November 12, 2018 14:33
@mary-poppins
Copy link
Copy Markdown

You can preview 16fa15f at https://pr26906-16fa15f.ngbuilds.io/.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, being less strict here resolves an issue for me as well 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great! I think I am done with refactoring this. Ready for a proper review.

@petebacondarwin petebacondarwin force-pushed the ngcc-private-declarations branch from 16fa15f to a75a204 Compare November 12, 2018 15:12
@petebacondarwin petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer effort2: days freq1: low target: major This PR is targeted for the next major release risk: low and removed state: WIP labels Nov 12, 2018
@mary-poppins
Copy link
Copy Markdown

You can preview a75a204 at https://pr26906-a75a204.ngbuilds.io/.

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

@alxhub, @JoostK & @gkalpak ready for a review. My big concern here is that there is no integration test of this functionality. I have checked that it works manually for me.

I would like to add a proper integration test, but I have been battling to move integration tests to use Bazel, and I think this is good enough to land now and then add the tests once Bazel tests are working properly.

@alxhub alxhub self-requested a review November 12, 2018 21:46
Copy link
Copy Markdown
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

I should have seen this PR earlier, as I should have used it as base commit (this badly conflicts with #27055, 😭)

Comment thread packages/compiler-cli/src/ngcc/src/analysis/private_declarations_analyzer.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/packages/transformer.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/rendering/esm_renderer.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/rendering/esm_renderer.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/rendering/renderer.ts Outdated
@mary-poppins
Copy link
Copy Markdown

You can preview ef5fafc at https://pr26906-ef5fafc.ngbuilds.io/.

@mary-poppins
Copy link
Copy Markdown

You can preview c9e8d33 at https://pr26906-c9e8d33.ngbuilds.io/.

IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
The `NgModuleDecoratorHandler` can now register all the references that
it finds in the `NgModule` metadata, such as `declarations`, `imports`,
`exports` etc.

This information can then be used by ngcc to work out if any of these
references are internal only and need to be manually exported from a
library's entry-point.

PR Close #26906
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
…ariables (#26906)

These variables are already declared as protected in the super class, and
so it is redundant to do it again in the subclasses.

PR Close #26906
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
There are a number of variables that need to be passed around
the program, in particular to the renderers, which benefit from being
stored in well defined objects.

The new `EntryPointBundle` structure is a specific format of an entry-point
and contains the compiled `BundleProgram` objects for the source and typings,
if appropriate.

This change helps with future refactoring, where we may need to add new
properties to this object. It allows us to maintain more stable APIs between
the constituent parts of ngcc, rather than passing lots of primitive values
around throughout the program.

PR Close #26906
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
This analyzer searches the source for declared classes that are not
exported publicly from the entry-point.

PR Close #26906
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
Ngcc will now render additional exports for classes that are referenced in
`NgModule` decorated classes, but which were not publicly exported
from an entry-point of the package.

This is important because when ngtsc compiles libraries processed by ngcc
it needs to be able to publcly access decorated classes that are referenced
by `NgModule` decorated classes in order to build templates that use these
classes.

Doing this re-exporting is not without its risks. There are chances that
the class is not exported correctly: there may already be similarly named
exports from the entry-point or the class may be being aliased. But there
is not much more we can do from the point of view of ngcc to workaround
such scenarios. Generally, packages should have been built so that this
approach works.

PR Close #26906
@petebacondarwin petebacondarwin deleted the ngcc-private-declarations branch December 1, 2018 16:27
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 effort2: days freq1: low merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low 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.

8 participants