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

Remove deprecated code in the router #18781

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@ocombe
Copy link
Contributor

ocombe commented Aug 18, 2017

PR Type

What kind of change does this PR introduce?

[ ] Other... Please describe: removing deprecated code

What is the current behavior?

The values true, false, legacy_enabled and legacy_disabled for the router parameter initialNavigation are deprecated.
RouterOutlet properties locationInjector and locationFactoryResolver are deprecated since v4.

What is the new behavior?

the values true, false for the router parameter initialNavigation have been removed as they were deprecated. Use enabled or disabled instead. The values legacy_enabled and legacy_disabled are still available but they will log a warning in dev mode and will be removed in v6.
RouterOutlet properties locationInjector and locationFactoryResolver have been removed as they were deprecated since v4.

Does this PR introduce a breaking change?

[x] Yes

@ocombe ocombe requested a review from vicb Aug 18, 2017

@googlebot googlebot added the cla: yes label Aug 18, 2017

@ocombe ocombe changed the title Remove router deprecated Remove deprecated code in the router Aug 18, 2017

@ocombe ocombe requested a review from jasonaden Aug 18, 2017

@@ -332,9 +322,8 @@ export class RouterInitializer {
const router = this.injector.get(Router);
const opts = this.injector.get(ROUTER_CONFIGURATION);

if (this.isLegacyDisabled(opts) || this.isLegacyEnabled(opts)) {
if (opts.initialNavigation === undefined) {
resolve(true);

This comment has been minimized.

Copy link
@vicb

vicb Aug 18, 2017

Contributor

Looks wrong to me - should be the same as enabled.
Also the API docs need to be update to state that enabled is the default.

@@ -372,26 +361,15 @@ export class RouterInitializer {
return;
}

if (this.isLegacyEnabled(opts)) {
if (opts.initialNavigation === undefined) {

This comment has been minimized.

Copy link
@vicb

vicb Aug 18, 2017

Contributor

same comment

@angular angular deleted a comment from mary-poppins Aug 21, 2017

@ocombe ocombe force-pushed the ocombe:remove-router-deprecated branch from ff83f1b to 189d7d7 Aug 21, 2017

@jasonaden
Copy link
Contributor

jasonaden left a comment

LGTM

@ocombe ocombe force-pushed the ocombe:remove-router-deprecated branch 3 times, most recently from 1026836 to d12247b Aug 21, 2017

@angular angular deleted a comment from mary-poppins Aug 22, 2017

@angular angular deleted a comment from mary-poppins Aug 22, 2017

@angular angular deleted a comment from mary-poppins Aug 22, 2017

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Aug 22, 2017

@mhevery mhevery closed this in d76761b Aug 22, 2017

mhevery added a commit that referenced this pull request Aug 22, 2017

refactor(router): remove deprecated `RouterOutlet` properties (#18781)
BREAKING CHANGE: `RouterOutlet` properties `locationInjector` and `locationFactoryResolver` have been removed as they were deprecated since v4.

PR Close #18781

@mhevery mhevery reopened this Aug 22, 2017

mhevery added a commit that referenced this pull request Aug 23, 2017

mhevery added a commit that referenced this pull request Aug 23, 2017

@ocombe

This comment has been minimized.

Copy link
Contributor Author

ocombe commented Aug 23, 2017

This PR changed the default behavior of the router. Most of the time the config option initialNavigation is undefined because there is no default value.

Before the change:

  • undefined, legacy_enabled and true had the same behavior: start the initial navigation before the preactivation
  • legacy_disabled and false would just skip the initial navigation
  • enabled would wait for preactivation, then run the preactivation hooks, and then start the initial navigation
  • disabled would wait for preactivation and then skip initial navigation

After the change:

  • legacy_enabled, legacy_disabled, true and false don't work anymore and should be changed to either enabled or disabled
  • undefined (which is the default value) now has the same behavior as enabled
  • enabled and disabled have the same behaviors as before

To understand the concept of preactivation, read the following design doc: https://docs.google.com/document/d/1Hlw1fPaVs-PCj5KPeJRKhrQGAvFOxdvTlwAcnZosu5A/edit

What does the change imply? Most of the time initialNavigation is undefined, which means that before the change it didn't run the preactivation before the initial navigation, and it didn't run the preactivation hooks either.
Now it does both of those, and if you get an error with a missing route it will just break and not load the website. That's what happened with one of the test in aio: the test asked protractor to load index.3.html, which would trigger an error in the app because the route index.3.html didn't exist (the base href was set to /, and the default route was empty: ''). But since it resolved anyway, it just redirected the app to the default route '' and it worked even with the error. After the change it just stopped at the error, I changed the base href to /index.3.html and the test was fixed. This change helped us discover this bug in the application where the error was just ignored, which was wrong. I don't know if there are other things that could break because of this, but I think that the previous behavior was wrong anyway.

@ocombe ocombe force-pushed the ocombe:remove-router-deprecated branch from d12247b to 6e8c01c Aug 23, 2017

@ocombe

This comment has been minimized.

Copy link
Contributor Author

ocombe commented Aug 23, 2017

@jasonaden I updated the first commit message, could you check if it's ok?

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Aug 23, 2017

@ocombe ocombe force-pushed the ocombe:remove-router-deprecated branch 2 times, most recently from 79bbd30 to d0e3cf9 Sep 1, 2017

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Sep 1, 2017

ocombe added some commits Aug 18, 2017

refactor(router): remove deprecated `initialNavigation` options
BREAKING CHANGE: the values `true`, `false` for the router parameter `initialNavigation` have been removed as they were deprecated. Use `enabled` or `disabled` instead. The values `legacy_enabled` and `legacy_disabled` are still available but they will log a warning in dev mode and will be removed in v6.
refactor(router): remove deprecated `RouterOutlet` properties
BREAKING CHANGE: `RouterOutlet` properties `locationInjector` and `locationFactoryResolver` have been removed as they were deprecated since v4.

@ocombe ocombe force-pushed the ocombe:remove-router-deprecated branch from d0e3cf9 to 869df0e Sep 1, 2017

@ocombe

This comment has been minimized.

Copy link
Contributor Author

ocombe commented Sep 1, 2017

I changed the behavior of this PR to help with the g3 migration: it no longer removes the legacy_enabled and legacy_disabled options, but the default behavior has still been changed to be enabled. Previously we both removed those options and changed the default behavior, forcing teams to update their code immediately.
Now they can set the value to legacy_enabled (which was the previous default) and update their code later. It will print a warning in dev mode to prompt them to update.

@ocombe ocombe requested a review from jasonaden Sep 1, 2017

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Sep 1, 2017

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Sep 1, 2017

This is an improvement, only one failure: http://test/OCL:167316942:BASE:167316961:1504297779950:584f06f0

@jasonaden can you look at this when you get a chance.

@matsko

This comment has been minimized.

Copy link
Member

matsko commented Sep 8, 2017

Landed as a9ef858

@matsko matsko closed this Sep 8, 2017

@ocombe ocombe deleted the ocombe:remove-router-deprecated branch Sep 11, 2017

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

refactor(router): remove deprecated `initialNavigation` options (angu…
…lar#18781)

BREAKING CHANGE: the values `true`, `false`, `legacy_enabled` and `legacy_disabled` for the router parameter `initialNavigation` have been removed as they were deprecated. Use `enabled` or `disabled` instead.

PR Close angular#18781

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

refactor(router): remove deprecated `RouterOutlet` properties (angula…
…r#18781)

BREAKING CHANGE: `RouterOutlet` properties `locationInjector` and `locationFactoryResolver` have been removed as they were deprecated since v4.

PR Close angular#18781

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.