feat(ivy): implement listing lazy routes in `ngtsc` by gkalpak · Pull Request #27697 · angular/angular

gkalpak · 2018-12-16T18:26:59Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

PR Type

Feature
Build related changes

What is the current behavior?

ngtsc does not support listLazyRoutes(), which the cli uses to split code into bundles.

Jira issue: FW-629

What is the new behavior?

ngtsc does support listing all lazy routes. It still does not support listing routes for a specific entry point (but I am not sure if it is necessary with Ivy - see "More information" below).

Does this PR introduce a breaking change?

Yes
No

More information

Listing lazy routes for specific entry points is not needed atm (but will probably be needed in the future). Created FW-860 to track that.

googlebot · 2018-12-16T18:27:05Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mary-poppins · 2018-12-16T18:32:53Z

You can preview 275069d at https://pr27697-275069d.ngbuilds.io/.

mary-poppins · 2018-12-16T19:58:01Z

You can preview 9211cb1 at https://pr27697-9211cb1.ngbuilds.io/.

mary-poppins · 2018-12-16T20:26:48Z

You can preview 2ac4ecc at https://pr27697-2ac4ecc.ngbuilds.io/.

mary-poppins · 2018-12-16T21:05:45Z

You can preview 045885b at https://pr27697-045885b.ngbuilds.io/.

mary-poppins · 2018-12-17T18:11:55Z

You can preview bff43e9 at https://pr27697-bff43e9.ngbuilds.io/.

mary-poppins · 2018-12-17T18:50:51Z

You can preview cf559d2 at https://pr27697-cf559d2.ngbuilds.io/.

mary-poppins · 2018-12-17T19:05:49Z

You can preview bed15e5 at https://pr27697-bed15e5.ngbuilds.io/.

mary-poppins · 2018-12-17T20:39:41Z

You can preview ccabee3 at https://pr27697-ccabee3.ngbuilds.io/.

petebacondarwin

I couldn't find anything wrong with the code.
I feel that analyzer is a bit of an overloaded term; is it sensible to use it here? Do other handlers have analyzer services?
I wonder if we should get ngtsc and ngcc to use shared code for module resolution? Or perhaps their structures are enough different.
I think the code would hugely benefit from some documentation comments and even some unit tests.
Otherwise LGTM!!

petebacondarwin · 2018-12-18T08:44:23Z

Can we document this enum and its members?

petebacondarwin · 2018-12-18T08:47:15Z

You could simplify a lot of the code below if you made this class DynamicValue<R = {}>, right?

+1 good idea.

petebacondarwin · 2018-12-18T08:56:06Z

Can we document this class? Who uses it and why is it needed?

Also should be using it elsewhere in ngtsc/ngcc? There is definitely module resolution going on in ngcc...

Added some docs. Looked at reusing this in ngcc and it doesnt seem to be a good fit (at least without a non-trivial refactoring, which feels out-of-scope for this PR).

petebacondarwin · 2018-12-18T13:25:29Z

Do we care if the identifier has more than one #?

Since this is only called with values of Route#loadChildren properties (for which having multiple # would be invalid), I think it is fine.

petebacondarwin · 2018-12-18T13:30:50Z

What FFR stands for is not documented - I am guessing it is Foreign Function Reference?

Close 😃
It is foreign function resolver, which can be used to statically resolve a call expression.
I'll add a comment and try to type it more explicitly 😃

petebacondarwin · 2018-12-18T13:31:45Z

This feels more like a register... than a resolve... method??

Or at least this method is actually doing two things, it is resolving the RouterEntryPoint and registering it so that it can be subsequently accessed via fromNgModule.

Yes, it will register it if it is not already registered (but it feels like a side effect).
Open to name suggestions 😁

alxhub

LGTM, pending Pete's comments :)

alxhub · 2018-12-19T19:56:46Z

Please add a comment with the name of the argument for which null is being passed here.

alxhub · 2018-12-19T19:57:41Z

+1 good idea.

googlebot · 2018-12-19T20:30:11Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

alxhub · 2018-12-19T20:40:40Z

https://test.corp.google.com/ui#id=OCL:226215551:BASE:226215725:1545251503759:9f5a4794

…propriate This makes the types (and intentions) more explicit and clear.

This commit introduces the NgModuleRouteAnalyzer & friends, which given metadata about the NgModules in a program can extract the list of lazy routes in the same format that the ngtools API uses.

This commit introduces a new mode for the NgtscTestEnvironment which builds the NgtscProgram and then asks for the list of lazy routes, instead of running the TS emit phase.

This commit uses the NgModuleRouteAnalyzer introduced previously to implement listLazyRoutes() for NgtscProgram. Currently this implementation is limited to listing routes globally and cannot list routes for a given lazy module. Testing seems to indicate that the CLI uses the global form, but this should be verified. Jira issue: FW-629

…ar/cli@7.2.1

… @angular/cli@7.2.1

googlebot · 2019-01-22T10:05:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ngbot · 2019-01-22T10:24:49Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "cla/google" is failing
     missing required labels: cla: yes
     forbidden labels detected: cla: no
     status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

gkalpak · 2019-01-22T10:26:27Z

merge-assistance:

Needs CLA yes (comits for @alxhub and me)
Pre-existing g3 failures (see feat(ivy): implement listing lazy routes in ngtsc #27697 (comment))

googlebot · 2019-01-22T17:57:52Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

alxhub · 2019-01-22T18:13:17Z

And another presubmit

PR Close #27697

…propriate (#27697) This makes the types (and intentions) more explicit and clear. PR Close #27697

This commit introduces the NgModuleRouteAnalyzer & friends, which given metadata about the NgModules in a program can extract the list of lazy routes in the same format that the ngtools API uses. PR Close #27697

angular-automatic-lock-bot · 2019-09-14T14:51:44Z

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

googlebot added the cla: no label Dec 16, 2018

gkalpak added feature Label used to distinguish feature request from other issues state: WIP target: major This PR is targeted for the next major release comp: ivy labels Dec 16, 2018

ngbot Bot added this to the needsTriage milestone Dec 16, 2018

gkalpak force-pushed the ngtsc-routes branch from 275069d to 9211cb1 Compare December 16, 2018 19:54

gkalpak force-pushed the ngtsc-routes branch from 9211cb1 to 2ac4ecc Compare December 16, 2018 20:20

gkalpak force-pushed the ngtsc-routes branch from bff43e9 to cf559d2 Compare December 17, 2018 18:46

gkalpak changed the title ~~WIP - feat(ivy): implement listing lazy routes in ngtsc~~ WIP - feat(ivy): implement listing routes in ngtsc Dec 17, 2018

gkalpak changed the title ~~WIP - feat(ivy): implement listing routes in ngtsc~~ WIP - feat(ivy): implement listing lazy routes in ngtsc Dec 17, 2018

gkalpak force-pushed the ngtsc-routes branch from bed15e5 to ccabee3 Compare December 17, 2018 20:35

gkalpak mentioned this pull request Dec 17, 2018

ivy+docs-infra: compile angular.io with Ivy #26947

Closed

11 of 13 tasks

gkalpak changed the title ~~WIP - feat(ivy): implement listing lazy routes in ngtsc~~ feat(ivy): implement listing lazy routes in ngtsc Dec 17, 2018

gkalpak requested a review from alxhub December 18, 2018 07:55

petebacondarwin approved these changes Dec 18, 2018

View reviewed changes

alxhub approved these changes Dec 19, 2018

View reviewed changes

alxhub added cla: yes and removed cla: no labels Dec 19, 2018

alxhub and others added 10 commits January 21, 2019 18:28

feat(ivy): create a ModuleResolver to map module paths to files

d971e46

refactor(ivy): re-use the ForeignFunctionResolver interface when ap…

0f76a1c

…propriate This makes the types (and intentions) more explicit and clear.

feat(ivy): implement ngtsc's route analysis

3915813

This commit introduces the NgModuleRouteAnalyzer & friends, which given metadata about the NgModules in a program can extract the list of lazy routes in the same format that the ngtools API uses.

test(ivy): introduce route testing mode for ngtsc tests

931638f

This commit introduces a new mode for the NgtscTestEnvironment which builds the NgtscProgram and then asks for the list of lazy routes, instead of running the TS emit phase.

build: upgrade cli-hello-world[-ivy] integration projects to @angul…

f3066de

…ar/cli@7.2.1

fixup! feat(ivy): DynamicValue now indicates why the value is dynamic

e6600b1

fixup! feat(ivy): create a ModuleResolver to map module paths to files

b1f61f8

fixup! feat(ivy): implement ngtsc's route analysis

641ee5b

fixup! build: upgrade cli-hello-world[-ivy] integration projects to…

96bf496

… @angular/cli@7.2.1

gkalpak force-pushed the ngtsc-routes branch from 8fc3b6d to 96bf496 Compare January 22, 2019 10:05

googlebot added cla: no and removed cla: yes labels Jan 22, 2019

alxhub added cla: yes and removed cla: no labels Jan 22, 2019

alxhub closed this in 9e5016c Jan 22, 2019

alxhub added a commit that referenced this pull request Jan 22, 2019

feat(ivy): create a ModuleResolver to map module paths to files (#27697)

19a2b78

PR Close #27697

alxhub pushed a commit that referenced this pull request Jan 22, 2019

refactor(ivy): re-use the ForeignFunctionResolver interface when ap…

2fc5f00

…propriate (#27697) This makes the types (and intentions) more explicit and clear. PR Close #27697

cexbrayat mentioned this pull request Jan 25, 2019

fix(ivy): scan simple children routes with no infinite recursion #28370

Closed

4 of 14 tasks

filipesilva mentioned this pull request Jan 29, 2019

fix(@ngtools/webpack): provide a TS-like path to ngProgram.listLazyRo… angular/angular-cli#13532

Merged

Conversation

gkalpak commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

More information

Uh oh!

googlebot commented Dec 16, 2018

Uh oh!

mary-poppins commented Dec 16, 2018

Uh oh!

mary-poppins commented Dec 16, 2018

Uh oh!

mary-poppins commented Dec 16, 2018

Uh oh!

mary-poppins commented Dec 16, 2018

Uh oh!

mary-poppins commented Dec 17, 2018

Uh oh!

mary-poppins commented Dec 17, 2018

Uh oh!

mary-poppins commented Dec 17, 2018

Uh oh!

mary-poppins commented Dec 17, 2018

Uh oh!

petebacondarwin 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!

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!

Uh oh!

Uh oh!

alxhub 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!

Uh oh!

googlebot commented Dec 19, 2018

Uh oh!

alxhub commented Dec 19, 2018

Uh oh!

googlebot commented Jan 22, 2019

Uh oh!

ngbot Bot commented Jan 22, 2019

Uh oh!

gkalpak commented Jan 22, 2019

Uh oh!

googlebot commented Jan 22, 2019

Uh oh!

alxhub commented Jan 22, 2019

Uh oh!

gkalpak commented Dec 16, 2018 •

edited

Loading