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

Hash routing opt in #667

Merged
merged 6 commits into from Jul 12, 2018
Merged

Hash routing opt in #667

merged 6 commits into from Jul 12, 2018

Conversation

tornqvist
Copy link
Member

@tornqvist tornqvist commented Jun 18, 2018

Resolves #666, resolves #440

Both newcomers as well as seasoned choo users keep stumbling over this and resolve to hackish workarounds or complex subclassing of choo.

Making hash routing opt-in, hash anchors will behave as expected as per browser defaults. By opting into hash routing hashes will be transformed to be a part of the pathname (this is the current default behavior).

Example with default new behavior: https://choo-hash-anchors.glitch.me
Example with hash routing enabled: https://choo-hash-anchors-enabled.glitch.me/my/app

I ended up factoring out nanolocation as the behavior had to be duplicated for location override.

This is most definitely a breaking change.

README.md Outdated
anchor links on the page is generally not recommended.
By default hashes are not taken into account when routing. Using hashes to
delimit routes (e.g. `/foo#bar`) is supported if the `hash` options set to
`true`. Regardles, when a hash is found we also check if there's an
Copy link
Contributor

Choose a reason for hiding this comment

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

missing the last s in regardless.

@toddself
Copy link
Contributor

This is a great idea. It'll need to be a semver major right?

@toddself toddself closed this Jun 18, 2018
@toddself toddself reopened this Jun 18, 2018
@toddself
Copy link
Contributor

Oops that dang close button is so close to the comment button :)

@YerkoPalma
Copy link
Member

I'm not clear why this would be a major change. In what case would it be breaking stuff?

@tornqvist
Copy link
Member Author

Since hash routing is enabled by default, changing it to be opt-in would break any app that currently relies on hashes being interpreted as part of the pathname.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

I think this approach is reasonable. iirc Choo v2 had a switch for this, which we removed for v3. I think it is indeed right to reintroduce. LGTM

@goto-bus-stop
Copy link
Member

How about adding the hash option with default true in a minor release and switching to false in v7?

@tornqvist
Copy link
Member Author

A minor version defaulting to true would be a great way of getting this out there asap. I’m currently on vacation but can resolve the conflict and change the default in a day or two.

@tornqvist
Copy link
Member Author

tornqvist commented Jul 12, 2018

I switched it around to default hash routing to true so that we can release this as a minor. Should revisit this, changing it to default to false with next major release (see #563). Merging this now.

@tornqvist tornqvist merged commit 6513f42 into choojs:master Jul 12, 2018
@tornqvist tornqvist deleted the hash-routing-opt-in branch July 12, 2018 09:29
@tornqvist
Copy link
Member Author

v6.13.0 🎉

tornqvist added a commit to tornqvist/choo that referenced this pull request Jun 8, 2019
* master: (71 commits)
  6.13.3
  Revert init order change
  Add browser tests (choojs#696)
  6.13.2
  Fix location missing on store init (choojs#695)
  Typo Fix in Readme (choojs#693)
  6.13.1
  Fix wrong this usage in nanohref integration (choojs#689)
  Some spelling & typo fixes in readme (choojs#688)
  Fix inspect script (choojs#654)
  Add v6 tests (choojs#674)
  Remove references to bel (choojs#678)
  6.13.0
  Add hash option (choojs#667)
  Add documentation on components (choojs#673)
  Update README.md (choojs#669)
  6.12.1
  6.12.0
  typings: add app to app.use cb (choojs#665)
  update dependencies (choojs#663)
  ...
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.

Page anchors ignored handling both hash routing & anchor links
5 participants