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

Fix URL creation with memory history #9814

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Jan 4, 2023

Fixing an oversight from https://github.com/remix-run/react-router/pull/9682/files#r1040159998. Now URL creation is driven by history.createURL so we have clear boundaries between memory versus browser cases.

Closes #9796

@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2023

🦋 Changeset detected

Latest commit: 94efa1a

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

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Patch
react-router Patch
react-router-dom 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 Jan 4, 2023 that may be closed by this pull request
@@ -1,7 +1,3 @@
/**
* @jest-environment ./__tests__/custom-environment.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure why we had 2 ways of doing this but I moved the custom environment stuff into setup.ts

Comment on lines +1 to +3
/**
* @jest-environment node
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because tests by default run in jsdom, we didn't catch this regression since window was defined during test runs when using a memory history. Now we have this standalone file that runs directly in node to catch those


if (!globalThis.AbortController) {
// @ts-expect-error
globalThis.AbortController = NodeAbortController;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed for the new node test run

globalThis.TextEncoder = NodeTextEncoder;
// @ts-expect-error
globalThis.TextDecoder = NodeTextDecoder;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied over from the deleted custom environment

@@ -1719,7 +1728,7 @@ export function createRouter(init: RouterInit): Router {

// Check if this an external redirect that goes to a new origin
if (typeof window?.location !== "undefined") {
let newOrigin = createClientSideURL(redirect.location).origin;
let newOrigin = init.history.createURL(redirect.location).origin;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All (non-static-handler) URL creation now uses the history.createURL function

return typeof to === "string" ? to : createPath(to);
createHref,
createURL(to) {
return new URL(createHref(to), "http://localhost");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

memory histories will use localhost as the URL base

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.

[Bug]: createMemoryRouter is dependant on Web APIs
3 participants