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

#3941 return decoded hash in getHash() #3955

Closed
wants to merge 1 commit into from
Closed

Conversation

bobmop
Copy link

@bobmop bobmop commented Feb 5, 2016

...to compare correctly to cached fragment in navigate()

compare correctly to cached fragment in navigate()
@akre54
Copy link
Collaborator

akre54 commented Feb 5, 2016

@braddunbar ?

@akre54 akre54 added the change label Feb 5, 2016
@braddunbar
Copy link
Collaborator

This seems fine to me. I'd say it could even be moved up into getFragment since getPath calls decodeFragment too.

@bobmop
Copy link
Author

bobmop commented Feb 8, 2016

The methods getPathand getHash are also called separately. I can't figure out the influence...

@megawac megawac mentioned this pull request May 6, 2016
@megawac
Copy link
Collaborator

megawac commented May 6, 2016

I'm fine with this but it may be a minor breaking change. I'd consider it a bug fix personally though

@barceyken
Copy link

We added this to our project too 👍

@haleo9000
Copy link

This seems to break browsers 'back' navigation button. However, it does fix the above stated problem.

@jashkenas
Copy link
Owner

This seems to break browsers 'back' navigation button. However, it does fix the above stated problem.

Uh.

@platinumazure
Copy link
Contributor

platinumazure commented Oct 27, 2016

It'd be great to have normalized hash decoding behavior, but the bit about breaking a browser's "back" button is highly concerning.

  • Have we established the exact impact (which browsers are affected)?
  • Does anyone know why that happens?
  • Is that issue fixable, or is this the wrong direction?

@Daniel-Alonso-Exabeam
Copy link

This issue is related to #4132
I have proposed my own solution in that thread, please take a look!

@jgonggrijp jgonggrijp added this to Low priority in Dusting off Dec 18, 2021
@jgonggrijp
Copy link
Collaborator

Rejecting this as it is a breaking change and it seems to cause worse problems than it aims to solve.

@jgonggrijp jgonggrijp closed this Jul 19, 2023
Dusting off automation moved this from Low priority to Closed Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants