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

fix(router-plugin): normalize URL by stripping base href #1178

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

arturovt
Copy link
Member

PR Checklist

PR Type

[x] Bugfix
[ ] 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?

Issue Number: #1174

What is the new behavior?

Location.normalize() strips base href from the provided URL

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@splincode
Copy link
Member

I still hope we have unit tests for this.

@markwhitfeld
Copy link
Member

@arturovt do you think it is possible to test this?

@arturovt
Copy link
Member Author

I really don't mind writing unit tests for that, but this is so unique a case... I find it difficult to answer, also we do a return there if Angular is in test mode as it will break all our tests, just because karma or jest goes to the /context.html URL first

@markwhitfeld
Copy link
Member

Ah, that is a good reason to not have tests.
We could work around it by mocking out the location with jest but that may get ugly??

@splincode
Copy link
Member

E2E test help you)

@arturovt
Copy link
Member Author

@splincode No. This is very unique case, this was done due to storage + router plugins combination issue.

@markwhitfeld markwhitfeld merged commit 680f755 into master Jul 26, 2019
@arturovt arturovt deleted the bugfix/1174 branch July 27, 2019 15:17
@markwhitfeld markwhitfeld added this to the 3.5.x milestone Aug 29, 2019
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.

None yet

3 participants