feat(core): add initialNavigation schematic #36926

CaerusKaru · 2020-05-05T00:44:49Z

Add a schematic to update users to the new v11 initialNavigation
options for RouterModule. This replaces the deprecated/removed
true, false, legacy_disabled, and legacy_enabled options
with the newer enabledBlocking and enabledNonBlocking options.

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

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

Follow-up to #33128

atscott · 2020-07-22T15:53:29Z

packages/core/schematics/migrations/initial-navigation/README.md

+those options by hand. Otherwise, the compiler will error if the types are sufficiently enforced.
+
+The basic migration strategy is as follows:
+* `legacy_disabled` || `false` => `disabled`


I'm not sure if we can have this change as part of the migration in 10.1 since legacy_disabled and false work slightly differently than disabled. We might only be able to make migrations that are exactly 1 to 1 now and then update this in v11.

Yeah, I'd agree with this.

@CaerusKaru now that master is targeting v11, I think we can submit this change. Can you do a quick pass over this migration and the follow-up PR to remove the old options?

devversion

LGTM. A few minor nits. I'm glad we decided on the more straightforward non-static-interpreter approach as that seems to capture the most common cases quite reliably.

devversion · 2020-07-22T19:01:30Z

packages/core/schematics/migrations/initial-navigation/README.md

+those options by hand. Otherwise, the compiler will error if the types are sufficiently enforced.
+
+The basic migration strategy is as follows:
+* `legacy_disabled` || `false` => `disabled`


Yeah, I'd agree with this.

packages/core/schematics/migrations/initial-navigation/collector.ts

packages/core/schematics/migrations/initial-navigation/google3/tslint_update_recorder.ts

devversion · 2020-07-22T19:11:26Z

packages/core/schematics/test/initial_navigation_migration_spec.ts

+
+    await runMigration();
+    expect(tree.readContent('/index.ts')).toContain(`enabledNonBlocking`);
+    expect(tree.readContent('/index.ts')).toContain(`disabled`);


Nit: should these assertions (and in other tests) be a little more precise? Right now options could theoretically swapped in wrong order. Might just make this a small regex checking for surrounding context.

atscott

LGTM with a couple small comments

packages/core/schematics/migrations/initial-navigation/transform.ts

packages/core/schematics/test/google3/initial_navigation_rule_spec.ts

gkalpak

Reviewed-for: dev-infra

packages/core/schematics/migrations.json

gkalpak · 2020-10-12T18:49:56Z

packages/core/schematics/test/google3/initial_navigation_rule_spec.ts

@@ -0,0 +1,211 @@
+/**


Nit: The _rule part in the file name feels confusing. I think it would be more consistent without the _rule part as well (although some other spec files have it, most don't).

gkalpak · 2020-10-12T18:53:07Z

packages/core/schematics/migrations/module-with-providers/transform.ts

@@ -20,7 +20,7 @@ export interface AnalysisFailure {
  message: string;
 }

-const TODO_COMMENT = 'TODO: The following node requires a generic type for `ModuleWithProviders';
+const TODO_COMMENT = 'TODO: The following node requires a generic type for `ModuleWithProviders`';


Nit: Since this change is unrelated, consider moving it to a separate commit.

packages/core/schematics/migrations/initial-navigation/BUILD.bazel

packages/core/schematics/migrations/initial-navigation/collector.ts

crisbeto · 2020-10-13T07:13:37Z

packages/core/schematics/migrations/google3/initialNavigationRule.ts

+    return failures;
+
+    /** Gets the update recorder for the specified source file. */
+    function getUpdateRecorder(sourceFile: ts.SourceFile): TslintUpdateRecorder {


Consider moving it into a private method or outside of applyWithProgram so the function doesn't get re-declared on each invocation.

packages/core/schematics/test/initial_navigation_migration_spec.ts

Add a schematic to update users to the new v11 `initialNavigation` options for `RouterModule`. This replaces the deprecated/removed `true`, `false`, `legacy_disabled`, and `legacy_enabled` options with the newer `enabledBlocking` and `enabledNonBlocking` options.

angular-automatic-lock-bot · 2020-11-14T16:23:27Z

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: yes label May 5, 2020

pullapprove bot requested review from josephperrott and kara May 5, 2020 00:44

mhevery added the area: core Issues related to the framework runtime label May 11, 2020

ngbot bot added this to the needsTriage milestone May 11, 2020

CaerusKaru force-pushed the adam/scm branch from 5c40864 to 504bc89 Compare July 21, 2020 18:45

pullapprove bot requested a review from devversion July 21, 2020 18:45

CaerusKaru force-pushed the adam/scm branch from 504bc89 to 81ae131 Compare July 21, 2020 18:46

CaerusKaru requested review from atscott and removed request for kara July 21, 2020 18:46

CaerusKaru added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Jul 21, 2020

CaerusKaru force-pushed the adam/scm branch 2 times, most recently from 6f23cbc to b3cb8ef Compare July 21, 2020 18:57

atscott reviewed Jul 22, 2020

View reviewed changes

devversion approved these changes Jul 22, 2020

View reviewed changes

devversion added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 29, 2020

pullapprove bot requested a review from alxhub September 28, 2020 20:30

CaerusKaru force-pushed the adam/scm branch from b3cb8ef to d73a492 Compare October 10, 2020 03:32

CaerusKaru removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 10, 2020

CaerusKaru assigned atscott Oct 10, 2020

CaerusKaru force-pushed the adam/scm branch 2 times, most recently from dd42af1 to e12d456 Compare October 10, 2020 03:53

atscott approved these changes Oct 10, 2020

View reviewed changes

packages/core/schematics/migrations/initial-navigation/transform.ts Outdated Show resolved Hide resolved

packages/core/schematics/test/google3/initial_navigation_rule_spec.ts Outdated Show resolved Hide resolved

CaerusKaru force-pushed the adam/scm branch 4 times, most recently from 68bffa3 to cfd6380 Compare October 10, 2020 05:19

gkalpak requested changes Oct 12, 2020

View reviewed changes

CaerusKaru force-pushed the adam/scm branch from cfd6380 to d11a25d Compare October 12, 2020 20:56

crisbeto reviewed Oct 13, 2020

View reviewed changes

crisbeto approved these changes Oct 13, 2020

View reviewed changes

atscott force-pushed the adam/scm branch 3 times, most recently from 44c03cd to d384e65 Compare October 13, 2020 20:51

gkalpak approved these changes Oct 14, 2020

View reviewed changes

atscott 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 Oct 14, 2020

atscott removed request for josephperrott and alxhub October 14, 2020 16:42

CaerusKaru and others added 2 commits October 14, 2020 11:21

fixup! feat(core): add initialNavigation schematic

2f8fcf7

atscott force-pushed the adam/scm branch from d384e65 to 2f8fcf7 Compare October 14, 2020 18:22

atscott closed this in 0ec7043 Oct 14, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 14, 2020

feat(core): add initialNavigation schematic #36926

feat(core): add initialNavigation schematic #36926

Uh oh!

Conversation

CaerusKaru commented May 5, 2020 • 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?

Other information

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!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gkalpak left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Nov 14, 2020

Uh oh!

Uh oh!

CaerusKaru commented May 5, 2020 •

edited

Loading