Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: update createHref to be history-aware #9409

Merged
merged 10 commits into from
Oct 17, 2022
Merged

Conversation

brophdawg11
Copy link
Contributor

Fixes #9392

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2022

🦋 Changeset detected

Latest commit: 3054115

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
react-router-dom Patch
@remix-run/router Patch
react-router Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 linked an issue Oct 5, 2022 that may be closed by this pull request
@brophdawg11 brophdawg11 force-pushed the brophdawg11/create-href-fix branch from b87b1e9 to 60553bf Compare October 6, 2022 15:25
createHref,
// Passthrough to history-aware createHref used by useHref so we get proper
// hash-aware URLs in DOM paths
createHref: (to: To) => init.history.createHref(to),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just an oversight. We have a local createHref brought over from transition manager that creates a "server" URL without the hash, but the one we want to expose is the history-aware version so that hash routers properly generate <a href="#/path> links

"<div>
<div>
<a
href=\\"/bar\\"
Copy link
Contributor Author

@brophdawg11 brophdawg11 Oct 13, 2022

Choose a reason for hiding this comment

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

Removed the href-specific parts of these snapshots since href-generation is tested elsewhere. This way we can share these functional tests between createBrowserRouter and createHashRouter. Hiding whitespace makes this easier to see 😄

@@ -2723,7 +2725,7 @@ function findRedirect(results: DataResult[]): RedirectResult | undefined {
}

// Create an href to represent a "server" URL without the hash
function createHref(location: Partial<Path> | Location | URL) {
function createServerHref(location: Partial<Path> | Location | URL) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this to avoid confusion

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

Successfully merging this pull request may close these issues.

[Bug]: href for NavLink in hashrouter does not use #
2 participants