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

only pass url to original history fn if it's a string #342

Closed
wants to merge 2 commits into from
Closed

only pass url to original history fn if it's a string #342

wants to merge 2 commits into from

Conversation

maartenverbaarschot
Copy link

Hi :)

At our company we started using Bugsnag last week, it helped us fix quite a few bugs in our code already. But today we discovered a bug that is on your side ;-). It is a quite severe one in Internet Explorer (11) that breaks Vue-Router, making our app as good as unusable for Internet Explorer users.

The symptom is that it immediately redirects our app to '/undefined', from there on the app router will get all confused.

I found out that bugsnag-js wraps window.history functions for breadcrumb purposes. Because of the wrapper, when calling replaceState without a URL, Internet Explorer will convert the URL to the string 'undefined'. This can be solved by only passing the URL parameter to the original function it it's a string.

There was a similar issue with Google Tag Manager some time ago, see this comment in another project for background: vuejs/vue-router#2023 (comment)

Fixes a bug in Internet Explorer where it converts `undefined` to a
string when passed, causing an unintended redirect to '/undefined'.
Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

Ah this looks like a nasty one! Thanks so much for providing a fix.

I'm just thinking about all of the other kinds of values that might get passed in (intentionally and unintentionally)…

I wonder if it might be better to be more permissive on the value that's passed in and only explicitly avoid it if it === undefined.

I can imagine a scenario where a number gets passed in and things "just" work because the number gets coerced, but with this change it would fail.

I think the call should look like:

orig.apply([ target, state, title ].concat(url !== undefined ? url : []) 

Let me know what you think!

@maartenverbaarschot
Copy link
Author

Your proposal is a nice improvement. Wouldn't want to break any other apps that currently rely on the number-to-string coersion :). I will add another commit to the PR in a moment

bengourley added a commit that referenced this pull request May 1, 2018
Only pass the url parameter to history methods when it is not `undefined`.

Fixes a bug in IE11 where it converts `undefined` to a string when passed,
causing an unintended redirect to '/undefined'.
@bengourley
Copy link
Contributor

Landed here cb80789

@bengourley bengourley closed this May 1, 2018
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.

2 participants