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

Restore [v5] HashRouter 'hashType' prop #8450

Closed
wants to merge 2 commits into from

Conversation

thejohnhoffer
Copy link

@thejohnhoffer thejohnhoffer commented Dec 7, 2021

Resolves #7703

Context

The v5 API page documenting HashRouter allows hashes to begin as # rather than #/. The v6 HashRouter no longer supports this because history@5 lost that property of history@4. Ten days ago, a contributor closed #7703. The contributor suggested the issue should be brought to the attention of with remix-run/history/, but noted "it was specifically removed in v5" of history while warning "it's not likely to return."

This is a request to replicate that feature of HashRouter that we lost in history@5 and react-router@6.

Solution

Only when <Router basename="">, this PR now supports To objects without a leading slash. Since the v5 docs claim that a "properly formatted basename should have a leading slash," this PR has no change to documented usage.

To support <HashState hashType="noslash">, that syntax is mapped to <Router basename="">.

So, the concept of a hashType is limited to the HashState constructor.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Dec 7, 2021

Hi @thejohnhoffer,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@thejohnhoffer thejohnhoffer changed the title Restore Restore [v5] HashRouter 'hashType' prop Dec 7, 2021
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Dec 7, 2021

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@timdorr
Copy link
Member

timdorr commented Dec 7, 2021

This isn't necessary. Just provide a basename="" prop to HashRouter and you have the same thing.

There were other modes of HashRouter/History that aren't covered by this PR, so it's not actually bringing back a compatible API. It's only solving for a specific case.

@timdorr timdorr closed this Dec 7, 2021
@jtojnar
Copy link

jtojnar commented Dec 9, 2021

Using basename="" will not work because normalizePathname will replace it with /:

let basename = normalizePathname(basenameProp);

const normalizePathname = (pathname: string): string =>
pathname.replace(/\/+$/, "").replace(/^\/*/, "/");

@thejohnhoffer
Copy link
Author

thejohnhoffer commented Dec 9, 2021

Using basename="" will not work because normalizePathname will replace it with /:
Exactly, @jtojnar!

Yes, I've noticed that as well. I've described this in issue #8459 and resolved it in PR #8460.

@thejohnhoffer
Copy link
Author

thejohnhoffer commented Dec 10, 2021

Issues #7703 and #8459 could now be addressed with PR #8463 and remix-run/history PR #911. My previous PR #8460 was closed, but I'm hopeful the new PRs will also be considered.

@thejohnhoffer
Copy link
Author

@jtojnar Thanks to react-router-dom@6.1.1, here's a quick workaround! It's possible the maintainers will feel history is a more appropriate place for this added feature. Even so, I know issue #912 is understandably not a high priority. Until then, a fork or wrapper can solve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants