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

Updates for Remix on RR 6.4 #9664

Merged
merged 29 commits into from Dec 15, 2022
Merged

Updates for Remix on RR 6.4 #9664

merged 29 commits into from Dec 15, 2022

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Dec 1, 2022

  • Changeset
  • Unit tests

@changeset-bot
Copy link

changeset-bot bot commented Dec 1, 2022

🦋 Changeset detected

Latest commit: 10b6c5b

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 Major
@remix-run/router Patch
react-router Major
react-router-dom-v5-compat Major
react-router-native Major

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

method: submitMethod,
replace,
relative,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same fix Remix used to ensure we allow <button type="submit" formmethod="post"> type overrides

// Shallow clone path so we can modify it below, otherwise we modify the
// object referenced by useMemo inside useResolvedPath
let path = { ...useResolvedPath(resolvedAction, { relative }) };
let path = { ...useResolvedPath(action ? action : ".", { relative }) };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove ?? usage which breaks older browsers

@@ -245,5 +245,5 @@ export function getFormSubmissionInfo(
let { protocol, host } = window.location;
let url = new URL(action, `${protocol}//${host}`);

return { url, method, encType, formData };
return { url, method: method.toLowerCase(), encType, formData };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lowercase incoming methods, since <Form method="POST"> is valid HTML

@@ -7,6 +7,7 @@
"__DEV__": true
},
"rules": {
"strict": 0
"strict": 0,
"no-restricted-syntax": ["error", "LogicalExpression[operator='??']"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See ?? removal below - added a lint rule to prevent that showing up again

@brophdawg11 brophdawg11 self-assigned this Dec 7, 2022
// location. If the user redirects from the action/loader this will be
// ignored and the redirect will be a PUSH
historyAction = HistoryAction.Replace;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryanflorence This fixes 2 things from the prior approach:

  • Explicit replace:false was respected if the action returned a redirect, but was getting lost if it didn't
  • Our "default to replace on submission" logic is now limited to same-path submissions, since you've already gone to the new location we don't need to replace the location you submitted from to avoid the double-back-click
    • /, link to /a, submit to /a -> history stack is [/, /a] - back button takes you to /
      • No changes here
    • /, link to /a, submit to /b -> history stack is [/, /a, /b] - back button takes you to /a
      • Previously this would have replaced /a and left you with [/, /b] and a back button would go back to /

@@ -435,6 +435,7 @@ type FetcherStates<TData = any> = {
formEncType: undefined;
formData: undefined;
data: TData | undefined;
" _hasFetcherDoneAnything "?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't love this but need to expose something for back-compat layer in Remix to differentiate idle/init and idle/done

}: {
getKey?: GetScrollRestorationKeyFunction;
storageKey?: string;
skip?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still not sure if this is 100% required or the best solution but going to play with it a bit more, need a way for Remix to communicate hydration info to skip initial restoration (which Remix did from an inline script)

Comment on lines +1654 to +1655
_isRedirect: true,
...(isFetchActionRedirect ? { _isFetchActionRedirect: true } : {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using location.state to drive back-compat useTransition.type layer in Remix

Comment on lines +2348 to +2352
// Add a null for all matched routes for proper revalidation on the client
loaderData: matches.reduce(
(acc, m) => Object.assign(acc, { [m.route.id]: null }),
{}
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure proper hydration by always sending null values for non-executed route loaders on the server. This is needed to handle differences where they may be no loader on the server, but there is a loader on the client (for route JS chunk fetching). The client gets confused on initial load and decides "this route is active but I've got no hydrated loader data so I need to do an initial fetch). It also impacts subsequent routing since it doesn't see this route as a revalidation.

Another approach would be for the client-side router to treat any hydrationData as complete and simply add null there for any active routes without hydrationData.

"@remix-run/router": patch
---

Fix explicit `replace` on submissions and `PUSH` on submission to new paths
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change implementation to be based on the next location (either from formAction or redirect). Replace if it's the same (pathname + search), push otherwise.

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