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

feat(routing): add option to disable stubs and use RouterTestingModule #188

Merged
merged 2 commits into from
Sep 20, 2019

Conversation

dirkluijk
Copy link
Collaborator

Closes #179.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

The ActivatedRouteStub does not work well with RouterTestingModule and Location.

Issue Number: 179

What is the new behavior?

This PR provides a feature toggle. Example in README.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@dirkluijk
Copy link
Collaborator Author

Before merging and releasing, should we create a beta/RC release to test if this PR really fixes the original issue?

@NetanelBasal
Copy link
Member

NetanelBasal commented Sep 19, 2019

@dirkluijk Yes, we can test it with the reproduction repo. It's in the original issue.

useFactory: () =>
new ActivatedRouteStub({
// this prevents the events property to be undefined
router.castToWritable().events = EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

What if someone needs to test the router events? They might register a subscription in the component and want to test it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, for now I assume that you should provide your own mock provider if you want to configure mocks more advanced.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we just do the following?

options.mockProvider(Router, {
  events = new Subject();
  emitRouterEvent(e) {
    this.events.next(e)
  }
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all right, will do

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, can you check again?

Copy link
Member

@NetanelBasal NetanelBasal left a comment

Choose a reason for hiding this comment

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

Great, thanks. Approved, just check the CI.

@dirkluijk dirkluijk merged commit 2fb7390 into master Sep 20, 2019
@dirkluijk dirkluijk deleted the feature/disable-routing-stubs branch September 20, 2019 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"NullInjectorError: No provider for Location!" when using createRoutingFactory
2 participants