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(compiler-cli): setComponentScope should only list used components/pipes #39662

Closed
wants to merge 2 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Nov 12, 2020

ngtsc will avoid emitting generated imports that would create an import
cycle in the user's program. The main way such imports can arise is when
a component would ordinarily reference its dependencies in its component
definition directiveDefs and pipeDefs. This requires adding imports,
which run the risk of creating a cycle.

When ngtsc detects that adding such an import would cause this to occur, it
instead falls back on a strategy called "remote scoping", where a side-
effectful call to setComponentScope in the component's NgModule file is
used to patch directiveDefs and pipeDefs onto the component. Since the
NgModule file already imports all of the component's dependencies (to
declare them in the NgModule), this approach does not risk adding a cycle.
It has several large downsides, however:

  1. it breaks under sideEffects: false logic in bundlers including the CLI
  2. it breaks tree-shaking for the given component and its dependencies

In particular, the impact on tree-shaking was exacerbated by the naive logic
ngtsc used to employ here. When this feature was implemented, at the time of
generating the side-effectful setComponentScope call, the compiler did not
know which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of directiveDefs in
the component definition itself, setComponentScope calls had to list the
entire compilation scope of the component's NgModule, including directives
and pipes which were not actually used in the template. This made the tree-
shaking impact much worse, since if the component's NgModule made use of any
shared NgModules (e.g. CommonModule), every declaration therein would
become un-treeshakable.

Today, ngtsc does have the information on which directives/pipes are
actually used in the template, but this was not being used during the remote
scoping operation. This commit modifies remote scoping to take advantage of
the extra context and only list used dependencies in setComponentScope
calls, which should ameliorate the tree-shaking impact somewhat.

@google-cla google-cla bot added the cla: yes label Nov 12, 2020
@alxhub alxhub marked this pull request as ready for review November 12, 2020 22:16
@alxhub alxhub added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 12, 2020
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Please consider linking to https://hackmd.io/Odw80D0pR6yfsOjg_7XCJg?view from the commit message as that design doc has a lot of context about the issue.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

This looks great! Let's add a link to the design doc from the commit, and maybe even from the code and let's get this in.

btw one location where it would be good to have more info about this problem and the design doc capturing it is is the definition of setComponentScope which currently has no docs. Adding a blurb and link there would help people unfamiliar with this issues.

Thanks for a quick turnaround on this PR!

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Reviewed-for: global-approvers

btw: the g3 failures seem unrelated.

…/pipes

ngtsc will avoid emitting generated imports that would create an import
cycle in the user's program. The main way such imports can arise is when
a component would ordinarily reference its dependencies in its component
definition `directiveDefs` and `pipeDefs`. This requires adding imports,
which run the risk of creating a cycle.

When ngtsc detects that adding such an import would cause this to occur, it
instead falls back on a strategy called "remote scoping", where a side-
effectful call to `setComponentScope` in the component's NgModule file is
used to patch `directiveDefs` and `pipeDefs` onto the component. Since the
NgModule file already imports all of the component's dependencies (to
declare them in the NgModule), this approach does not risk adding a cycle.
It has several large downsides, however:

1. it breaks under `sideEffects: false` logic in bundlers including the CLI
2. it breaks tree-shaking for the given component and its dependencies

See this doc for further details: https://hackmd.io/Odw80D0pR6yfsOjg_7XCJg?view

In particular, the impact on tree-shaking was exacerbated by the naive logic
ngtsc used to employ here. When this feature was implemented, at the time of
generating the side-effectful `setComponentScope` call, the compiler did not
know which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of `directiveDefs` in
the component definition itself, `setComponentScope` calls had to list the
_entire_ compilation scope of the component's NgModule, including directives
and pipes which were not actually used in the template. This made the tree-
shaking impact much worse, since if the component's NgModule made use of any
shared NgModules (e.g. `CommonModule`), every declaration therein would
become un-treeshakable.

Today, ngtsc does have the information on which directives/pipes are
actually used in the template, but this was not being used during the remote
scoping operation. This commit modifies remote scoping to take advantage of
the extra context and only list used dependencies in `setComponentScope`
calls, which should ameliorate the tree-shaking impact somewhat.
`setComponentScope` was previously undocumented. This commit adds a short
explanation of what the function does, and adds a link to a doc which
explains issues with cycles in more detail.
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 13, 2020
atscott pushed a commit that referenced this pull request Nov 13, 2020
…/pipes (#39662)

ngtsc will avoid emitting generated imports that would create an import
cycle in the user's program. The main way such imports can arise is when
a component would ordinarily reference its dependencies in its component
definition `directiveDefs` and `pipeDefs`. This requires adding imports,
which run the risk of creating a cycle.

When ngtsc detects that adding such an import would cause this to occur, it
instead falls back on a strategy called "remote scoping", where a side-
effectful call to `setComponentScope` in the component's NgModule file is
used to patch `directiveDefs` and `pipeDefs` onto the component. Since the
NgModule file already imports all of the component's dependencies (to
declare them in the NgModule), this approach does not risk adding a cycle.
It has several large downsides, however:

1. it breaks under `sideEffects: false` logic in bundlers including the CLI
2. it breaks tree-shaking for the given component and its dependencies

See this doc for further details: https://hackmd.io/Odw80D0pR6yfsOjg_7XCJg?view

In particular, the impact on tree-shaking was exacerbated by the naive logic
ngtsc used to employ here. When this feature was implemented, at the time of
generating the side-effectful `setComponentScope` call, the compiler did not
know which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of `directiveDefs` in
the component definition itself, `setComponentScope` calls had to list the
_entire_ compilation scope of the component's NgModule, including directives
and pipes which were not actually used in the template. This made the tree-
shaking impact much worse, since if the component's NgModule made use of any
shared NgModules (e.g. `CommonModule`), every declaration therein would
become un-treeshakable.

Today, ngtsc does have the information on which directives/pipes are
actually used in the template, but this was not being used during the remote
scoping operation. This commit modifies remote scoping to take advantage of
the extra context and only list used dependencies in `setComponentScope`
calls, which should ameliorate the tree-shaking impact somewhat.

PR Close #39662
atscott pushed a commit that referenced this pull request Nov 13, 2020
`setComponentScope` was previously undocumented. This commit adds a short
explanation of what the function does, and adds a link to a doc which
explains issues with cycles in more detail.

PR Close #39662
@atscott atscott closed this in c59f401 Nov 13, 2020
atscott pushed a commit that referenced this pull request Nov 13, 2020
`setComponentScope` was previously undocumented. This commit adds a short
explanation of what the function does, and adds a link to a doc which
explains issues with cycles in more detail.

PR Close #39662
basherr pushed a commit to basherr/angular that referenced this pull request Nov 14, 2020
…/pipes (angular#39662)

ngtsc will avoid emitting generated imports that would create an import
cycle in the user's program. The main way such imports can arise is when
a component would ordinarily reference its dependencies in its component
definition `directiveDefs` and `pipeDefs`. This requires adding imports,
which run the risk of creating a cycle.

When ngtsc detects that adding such an import would cause this to occur, it
instead falls back on a strategy called "remote scoping", where a side-
effectful call to `setComponentScope` in the component's NgModule file is
used to patch `directiveDefs` and `pipeDefs` onto the component. Since the
NgModule file already imports all of the component's dependencies (to
declare them in the NgModule), this approach does not risk adding a cycle.
It has several large downsides, however:

1. it breaks under `sideEffects: false` logic in bundlers including the CLI
2. it breaks tree-shaking for the given component and its dependencies

See this doc for further details: https://hackmd.io/Odw80D0pR6yfsOjg_7XCJg?view

In particular, the impact on tree-shaking was exacerbated by the naive logic
ngtsc used to employ here. When this feature was implemented, at the time of
generating the side-effectful `setComponentScope` call, the compiler did not
know which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of `directiveDefs` in
the component definition itself, `setComponentScope` calls had to list the
_entire_ compilation scope of the component's NgModule, including directives
and pipes which were not actually used in the template. This made the tree-
shaking impact much worse, since if the component's NgModule made use of any
shared NgModules (e.g. `CommonModule`), every declaration therein would
become un-treeshakable.

Today, ngtsc does have the information on which directives/pipes are
actually used in the template, but this was not being used during the remote
scoping operation. This commit modifies remote scoping to take advantage of
the extra context and only list used dependencies in `setComponentScope`
calls, which should ameliorate the tree-shaking impact somewhat.

PR Close angular#39662
basherr pushed a commit to basherr/angular that referenced this pull request Nov 14, 2020
…r#39662)

`setComponentScope` was previously undocumented. This commit adds a short
explanation of what the function does, and adds a link to a doc which
explains issues with cycles in more detail.

PR Close angular#39662
@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 Dec 14, 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 cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants