fix(router): properly assign ExtraOptions to Router in RouterTestingModule by zelliott · Pull Request #39096 · angular/angular

zelliott · 2020-10-02T16:01:18Z

Previously, RouterTestingModule only assigned two of the options within ExtraOptions to the router. Now, it assigns the same options as RouterModule does (with the exception of enableTracing at the request of @atscott) via a new shared function assignExtraOptionsToRouter.

Fixes #23347

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?

Setting many of the options within ExtraOptions in the config for RouterTestingModule did not actually update the router's config (e.g. onSameUrlNavigation).

Issue Number: #23347

What is the new behavior?

The same options can now be properly set on both RouterModule and RouterTestingModule (with the exception of enableTracing).

Does this PR introduce a breaking change?

Yes
No

Other information

atscott

Hi @zelliott, thanks for the PR. As mentioned in the last comment on the bug, there are actually several options that don't make it from ExtraOptions to the router when using RouterTestingModule. I think we should address those as well.

It might be a good idea to create a function in router_module like assignExtraOptionsToRouterFields(router: Router, options: ExtraOptions) { ... } and share the code for the RouterModule implementation and the RouterTestingModule

angular/packages/router/src/router_module.ts

Lines 443 to 483 in db56cf1

    
           if (urlHandlingStrategy) { 
        
             router.urlHandlingStrategy = urlHandlingStrategy; 
        
           } 
        
           if (routeReuseStrategy) { 
        
             router.routeReuseStrategy = routeReuseStrategy; 
        
           } 
        
           if (opts.errorHandler) { 
        
             router.errorHandler = opts.errorHandler; 
        
           } 
        
           if (opts.malformedUriErrorHandler) { 
        
             router.malformedUriErrorHandler = opts.malformedUriErrorHandler; 
        
           } 
        
           if (opts.enableTracing) { 
        
             const dom = getDOM(); 
        
             router.events.subscribe((e: Event) => { 
        
               dom.logGroup(`Router Event: ${(<any>e.constructor).name}`); 
        
               dom.log(e.toString()); 
        
               dom.log(e); 
        
               dom.logGroupEnd(); 
        
             }); 
        
           } 
        
           if (opts.onSameUrlNavigation) { 
        
             router.onSameUrlNavigation = opts.onSameUrlNavigation; 
        
           } 
        
           if (opts.paramsInheritanceStrategy) { 
        
             router.paramsInheritanceStrategy = opts.paramsInheritanceStrategy; 
        
           } 
        
           if (opts.urlUpdateStrategy) { 
        
             router.urlUpdateStrategy = opts.urlUpdateStrategy; 
        
           } 
        
           if (opts.relativeLinkResolution) { 
        
             router.relativeLinkResolution = opts.relativeLinkResolution; 
        
           }

(excluding enableTracing)

zelliott · 2020-10-03T16:43:25Z

As mentioned in the last comment on the bug, there are actually several options that don't make it from ExtraOptions to the router when using RouterTestingModule. I think we should address those as well.

Done, PTAL.

atscott

One small nit on the test organization but otherwise LGTM. presubmit. Starting global as well.

We might want to consider adding a breaking change note even though it's pretty unlikely/rare that this would break people. Something like
"BREAKING CHANGE: The RouterTestingModule now correctly assigns errorHandler, urlUpdateSstraategy, (...etc) from the ExtraOptions to the test module rather than just malformedUriErrorHandler and paramsInheritanceStrategy. If your test is setting these options, they will now be applied to navigations where they were ignored before and could result in a change in test behavior."

IgorMinar

LGTM, thank you

…odule Previously, RouterTestingModule only assigned two of the options within ExtraOptions to the Router. Now, it assigns the same options as RouterModule does (with the exception of enableTracing) via a new shared function assignExtraOptionsToRouter. Fixes angular#23347

atscott · 2020-10-05T23:19:00Z

global presubmit shows no related failures (1 test failed, but it's already failing at head). Marking as merge ready.

angular-automatic-lock-bot · 2020-11-05T16:38:05Z

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 Oct 2, 2020

pullapprove Bot requested review from IgorMinar and atscott October 2, 2020 16:01

zelliott force-pushed the router branch from 7ee05ce to 95ffcee Compare October 2, 2020 16:22

atscott requested changes Oct 2, 2020

View reviewed changes

atscott added area: router target: patch This PR is targeted for the next patch release labels Oct 2, 2020

ngbot Bot modified the milestone: needsTriage Oct 2, 2020

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

zelliott force-pushed the router branch from 95ffcee to 5e3d55a Compare October 3, 2020 15:21

zelliott changed the title ~~fix(router) onSameUrlNavigation config is now passed through RouterTestingModule~~ fix(router): properly assign ExtraOptions to Router in RouterTestingModule Oct 3, 2020

zelliott force-pushed the router branch 3 times, most recently from 0d8595d to 320c44a Compare October 3, 2020 16:21

zelliott requested a review from atscott October 3, 2020 16:43

atscott approved these changes Oct 5, 2020

View reviewed changes

Comment thread packages/router/test/integration.spec.ts Outdated

IgorMinar approved these changes Oct 5, 2020

View reviewed changes

zelliott force-pushed the router branch from 320c44a to 4fe54b7 Compare October 5, 2020 19:07

zelliott force-pushed the router branch from 4fe54b7 to 6f5b680 Compare October 5, 2020 19:08

atscott added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Oct 5, 2020

josephperrott closed this in d8c0534 Oct 5, 2020

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

Navigation Menu

Repository navigation

fix(router): properly assign ExtraOptions to Router in RouterTestingModule - #39096

fix(router): properly assign ExtraOptions to Router in RouterTestingModule#39096
zelliott wants to merge 1 commit into
angular:masterfrom
zelliott:router

zelliott commented Oct 2, 2020 •

edited

Loading

Uh oh!

atscott left a comment

Uh oh!

zelliott commented Oct 3, 2020

Uh oh!

atscott left a comment

Uh oh!

Uh oh!

IgorMinar left a comment

Uh oh!

atscott commented Oct 5, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (urlHandlingStrategy) {
	router.urlHandlingStrategy = urlHandlingStrategy;
	}

	if (routeReuseStrategy) {
	router.routeReuseStrategy = routeReuseStrategy;
	}

	if (opts.errorHandler) {
	router.errorHandler = opts.errorHandler;
	}

	if (opts.malformedUriErrorHandler) {
	router.malformedUriErrorHandler = opts.malformedUriErrorHandler;
	}

	if (opts.enableTracing) {
	const dom = getDOM();
	router.events.subscribe((e: Event) => {
	dom.logGroup(`Router Event: ${(<any>e.constructor).name}`);
	dom.log(e.toString());
	dom.log(e);
	dom.logGroupEnd();
	});
	}

	if (opts.onSameUrlNavigation) {
	router.onSameUrlNavigation = opts.onSameUrlNavigation;
	}

	if (opts.paramsInheritanceStrategy) {
	router.paramsInheritanceStrategy = opts.paramsInheritanceStrategy;
	}

	if (opts.urlUpdateStrategy) {
	router.urlUpdateStrategy = opts.urlUpdateStrategy;
	}

	if (opts.relativeLinkResolution) {
	router.relativeLinkResolution = opts.relativeLinkResolution;
	}

Conversation

zelliott commented Oct 2, 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!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

zelliott commented Oct 3, 2020

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

atscott commented Oct 5, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zelliott commented Oct 2, 2020 •

edited

Loading