Cyclic import detection in ngtsc / for g3 by alxhub · Pull Request #28169 · angular/angular

alxhub · 2019-01-16T00:53:13Z

No description provided.

mary-poppins · 2019-01-16T00:57:51Z

You can preview 2e5fe41 at https://pr28169-2e5fe41.ngbuilds.io/.

JoostK

Looks great, some minor things.

JoostK · 2019-01-16T08:02:18Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

Let's use _isCyclicImport 😄

Hahaha, fixed!

JoostK · 2019-01-16T08:03:08Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

I don't think this early return is correct here, but once you use _isCyclicImport the problem resolves itself

JoostK · 2019-01-16T08:04:39Z

packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts

Why isn't directives simply mapped from scope.directives? There's no initial content to deal with so that should work just fine.

Good point.

JoostK · 2019-01-16T08:05:06Z

packages/compiler-cli/src/ngtsc/cycles/test/util.ts

I'm actually debating whether I like this. It gets really annoying to write out example programs all the time, but having actual code to look at when you're debugging is nice. In this case I think it's okay because the programs being generated are very specific.

JoostK · 2019-01-16T08:06:12Z

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

This shouldn't be pure, or all the hard work for nothing ;)

Hahahaha. Good point! I'll actually invert the test so that we expect not to have a pure call there.

petebacondarwin · 2019-01-16T08:26:27Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

Swapping these two if and else blocks would remove the need for the negation on !cycleDetected.

I actually wrote it this way on purpose - the common case is the if, the uncommon case is the else.

/shrug/ :-)

petebacondarwin · 2019-01-16T08:29:25Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

This return here exits the whole resolve() method.
Are you saying that if a directive is an external expression but has no import we abort all resolving activity?

Nope, as @JoostK pointed out this flow only makes sense within the _isCyclicImport function.

petebacondarwin · 2019-01-16T08:32:24Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

This method does not appear to be used...

Ahah, I just realised that this is a WIP. The code above in the resolve method was copied from here, which is why there are the confusing return statements there.

mary-poppins · 2019-01-16T19:28:07Z

You can preview 5577728 at https://pr28169-5577728.ngbuilds.io/.

JoostK · 2019-01-16T19:41:17Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

nit:

const cycleDetected = scope.directives.some(meta => this._isCyclicImport(meta.directive, origin)) || Array.from(scope.pipes.values()).some(pipe => this._isCyclicImport(pipe, origin));

Done, nice idea.

JoostK · 2019-01-16T19:41:40Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

The comment has become a bit inaccurate ;)

mary-poppins · 2019-01-16T19:51:29Z

You can preview 1e7bfee at https://pr28169-1e7bfee.ngbuilds.io/.

JoostK

The CI fails because of the incorrect private export, which indicates a surprising dependency on this feature within the Angular Router codebase. That's ok for now, let me look into refactoring the Router to avoid the cyclic dep in Ivy builds sometime.

JoostK · 2019-01-16T20:00:41Z

packages/core/src/core_render3_private_export.ts

Whoopsie, caught by failing CI :)

mary-poppins · 2019-01-16T21:09:56Z

You can preview d5867b3 at https://pr28169-d5867b3.ngbuilds.io/.

mary-poppins · 2019-01-16T21:44:53Z

You can preview 20bf80b at https://pr28169-20bf80b.ngbuilds.io/.

mary-poppins · 2019-01-16T22:40:48Z

You can preview 5179313 at https://pr28169-5179313.ngbuilds.io/.

AndrewKushnir · 2019-01-17T18:07:20Z

packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts

nit: may be use map here (and for directives too)?

const pipes: Expression[] = scope.pipes.map(pipe => pipe.toExpression(context) !);

AndrewKushnir

LGTM 👍

This commit implements a cycle detector which looks at the import graph of TypeScript programs and can determine whether the addition of an edge is sufficient to create a cycle. As part of the implementation, module name to source file resolution is implemented via a ModuleResolver, using TS APIs.

By its nature, Ivy alters the import graph of a TS program, adding imports where template dependencies exist. For example, if ComponentA uses PipeB in its template, Ivy will insert an import of PipeB into the file in which ComponentA is declared. Any insertion of an import into a program has the potential to introduce a cycle into the import graph. If for some reason the file in which PipeB is declared imports the file in which ComponentA is declared (maybe it makes use of a service or utility function that happens to be in the same file as ComponentA) then this could create an import cycle. This turns out to happen quite regularly in larger Angular codebases. TypeScript and the Ivy runtime have no issues with such cycles. However, other tools are not so accepting. In particular the Closure Compiler is very anti-cycle. To mitigate this problem, it's necessary to detect when the insertion of an import would create a cycle. ngtsc can then use a different strategy, known as "remote scoping", instead of directly writing a reference from one component to another. Under remote scoping, a function 'setComponentScope' is called after the declaration of the component's module, which does not require the addition of new imports. FW-647 #resolve

This commit implements a cycle detector which looks at the import graph of TypeScript programs and can determine whether the addition of an edge is sufficient to create a cycle. As part of the implementation, module name to source file resolution is implemented via a ModuleResolver, using TS APIs. PR Close #28169

…ed (#28169) By its nature, Ivy alters the import graph of a TS program, adding imports where template dependencies exist. For example, if ComponentA uses PipeB in its template, Ivy will insert an import of PipeB into the file in which ComponentA is declared. Any insertion of an import into a program has the potential to introduce a cycle into the import graph. If for some reason the file in which PipeB is declared imports the file in which ComponentA is declared (maybe it makes use of a service or utility function that happens to be in the same file as ComponentA) then this could create an import cycle. This turns out to happen quite regularly in larger Angular codebases. TypeScript and the Ivy runtime have no issues with such cycles. However, other tools are not so accepting. In particular the Closure Compiler is very anti-cycle. To mitigate this problem, it's necessary to detect when the insertion of an import would create a cycle. ngtsc can then use a different strategy, known as "remote scoping", instead of directly writing a reference from one component to another. Under remote scoping, a function 'setComponentScope' is called after the declaration of the component's module, which does not require the addition of new imports. FW-647 #resolve PR Close #28169

…angular#28169) PR Close angular#28169

This commit implements a cycle detector which looks at the import graph of TypeScript programs and can determine whether the addition of an edge is sufficient to create a cycle. As part of the implementation, module name to source file resolution is implemented via a ModuleResolver, using TS APIs. PR Close angular#28169

…ed (angular#28169) By its nature, Ivy alters the import graph of a TS program, adding imports where template dependencies exist. For example, if ComponentA uses PipeB in its template, Ivy will insert an import of PipeB into the file in which ComponentA is declared. Any insertion of an import into a program has the potential to introduce a cycle into the import graph. If for some reason the file in which PipeB is declared imports the file in which ComponentA is declared (maybe it makes use of a service or utility function that happens to be in the same file as ComponentA) then this could create an import cycle. This turns out to happen quite regularly in larger Angular codebases. TypeScript and the Ivy runtime have no issues with such cycles. However, other tools are not so accepting. In particular the Closure Compiler is very anti-cycle. To mitigate this problem, it's necessary to detect when the insertion of an import would create a cycle. ngtsc can then use a different strategy, known as "remote scoping", instead of directly writing a reference from one component to another. Under remote scoping, a function 'setComponentScope' is called after the declaration of the component's module, which does not require the addition of new imports. FW-647 #resolve PR Close angular#28169

angular-automatic-lock-bot · 2019-09-14T15:22:22Z

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

alxhub requested review from a team January 16, 2019 00:53

googlebot added the cla: yes label Jan 16, 2019

alxhub requested a review from mhevery January 16, 2019 00:53

JoostK reviewed Jan 16, 2019

View reviewed changes

petebacondarwin reviewed Jan 16, 2019

View reviewed changes

alxhub force-pushed the ngtsc-cyclic-imports branch from 2e5fe41 to 5577728 Compare January 16, 2019 19:23

alxhub added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 16, 2019

JoostK reviewed Jan 16, 2019

View reviewed changes

alxhub force-pushed the ngtsc-cyclic-imports branch from 5577728 to 1e7bfee Compare January 16, 2019 19:46

JoostK reviewed Jan 16, 2019

View reviewed changes

packages/core/src/core_render3_private_export.ts Outdated

Copy link

Member

JoostK Jan 16, 2019

Choose a reason for hiding this comment

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

Whoopsie, caught by failing CI :)

alxhub force-pushed the ngtsc-cyclic-imports branch from 1e7bfee to d5867b3 Compare January 16, 2019 21:05

alxhub force-pushed the ngtsc-cyclic-imports branch from d5867b3 to 20bf80b Compare January 16, 2019 21:40

alxhub force-pushed the ngtsc-cyclic-imports branch from 20bf80b to 5179313 Compare January 16, 2019 22:36

AndrewKushnir reviewed Jan 17, 2019

View reviewed changes

alxhub removed the request for review from mhevery January 18, 2019 21:33

AndrewKushnir approved these changes Jan 22, 2019

View reviewed changes

alxhub force-pushed the ngtsc-cyclic-imports branch 3 times, most recently from 9804468 to 230c30e Compare January 23, 2019 18:47

jasonaden added the area: core Issues related to the framework runtime label Jan 24, 2019

ngbot bot added this to the needsTriage milestone Jan 24, 2019

alxhub added 2 commits January 25, 2019 11:52

test(ivy): clean up two parameters in ngtsc_spec.ts inferred as 'any'

09f8ef4

alxhub force-pushed the ngtsc-cyclic-imports branch from 230c30e to 2d633bd Compare January 25, 2019 20:07

alxhub force-pushed the ngtsc-cyclic-imports branch from 2d633bd to 34b87a1 Compare January 25, 2019 20:59

jasonaden closed this in a789a3f Jan 28, 2019

vetom pushed a commit to vetom/angular that referenced this pull request Jan 31, 2019

test(ivy): clean up two parameters in ngtsc_spec.ts inferred as 'any' (…

143e489

…angular#28169) PR Close angular#28169

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

alxhub commented Jan 16, 2019

Uh oh!

mary-poppins commented Jan 16, 2019

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jan 16, 2019

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jan 16, 2019

Uh oh!

mary-poppins commented Jan 16, 2019

Uh oh!

mary-poppins commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

alxhub Jan 16, 2019 •

edited

Loading