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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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!
There was a problem hiding this 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.
91686fb
to
99f93cb
Compare
`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.
…/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
`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
`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
…/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
…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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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
andpipeDefs
. 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 isused to patch
directiveDefs
andpipeDefs
onto the component. Since theNgModule 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:
sideEffects: false
logic in bundlers including the CLIIn 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 notknow which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of
directiveDefs
inthe component definition itself,
setComponentScope
calls had to list theentire 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 wouldbecome 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.