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

ref(js): Use useLocation in useUrlParams #70653

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evanpurkhiser
Copy link
Member

Newer versions of the history library do not support this function 0.
Use useLocation in this case instead.

@evanpurkhiser evanpurkhiser requested review from ryan953 and a team May 10, 2024 14:51
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 10, 2024
ryan953
ryan953 previously approved these changes May 10, 2024
Copy link

codecov bot commented May 10, 2024

Bundle Report

Changes will decrease total bundle size by 9 bytes ⬇️

Bundle name Size Change
app-webpack-bundle-array-push 26.77MB 9 bytes ⬇️

scttcper
scttcper previously approved these changes May 10, 2024
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/ref-js-use-uselocation-in-useurlparams branch from 6d37e88 to 11b2876 Compare May 10, 2024 15:05
@evanpurkhiser evanpurkhiser requested a review from a team as a code owner May 10, 2024 15:05
Newer versions of the history library do not support this function [0].
Use `useLocation` in this case instead.

[0]: https://github.com/remix-run/history/blob/485ebc177c1f3f8eef93b0d654fffd1d321c2ecd/packages/history/index.ts#L188
@evanpurkhiser
Copy link
Member Author

This does appear to break things in a weird way.

@evanpurkhiser evanpurkhiser dismissed stale reviews from scttcper and ryan953 May 17, 2024 05:38

it's broken

@evanpurkhiser
Copy link
Member Author

evanpurkhiser commented May 17, 2024

Okay so this is only broken in places where we're making synchronous calls to multiple uses of this hook

Specifically here

const handleSort = useCallback(
(fieldName: keyof typeof SortStrategies) => {
if (sortConfig.by === fieldName) {
setSortAsc(sortConfig.asc ? 'false' : 'true');
} else {
setSortAsc('true');
setSortBy(fieldName);
}
setDetailRow('');
},
[sortConfig, setSortAsc, setSortBy, setDetailRow]
);

We can't do this when using useLocation because each subsequent call will not have the new value of the location object, so it will remove values updated in the query string by the previous calls.

Not sure what the right fix is yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants