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 useLocation to receive 0 instead of null when state is set to 0 #11495

Merged
merged 11 commits into from May 8, 2024

Conversation

jungwoo3490
Copy link

@jungwoo3490 jungwoo3490 commented Apr 24, 2024

Fixes #11448

I fixed location's state property value like this.

let location: Location = {
  pathname: locationProp.pathname || "/",
  search: locationProp.search || "",
  hash: locationProp.hash || "",
  state: locationProp.state ?? null, // fixed
  key: locationProp.key || "default",
};

Now, useLocation receive 0 instead of null when state is set to 0.

Copy link

changeset-bot bot commented Apr 24, 2024

🦋 Changeset detected

Latest commit: adaeedf

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

This PR includes changesets to release 4 packages
Name Type
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router 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

@emrerdem1
Copy link

emrerdem1 commented Apr 25, 2024

Wouldn't it be better to use the nullish coalescing operator here?

 state: locationProp.state ?? null,

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

@jungwoo3490
Copy link
Author

Wouldn't it be better to use the nullish coalescing operator here?

 state: locationProp.state ?? null,

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

Thanks!! I'll apply it :)

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Apr 25, 2024

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

@jungwoo3490
Copy link
Author

In my opinion this is quite simple Pull Request, review this please
@ryanflorence @timdorr @brophdawg11

@brophdawg11
Copy link
Contributor

Could you add a unit test to packages/react-router-dom/__tests__/static-location-test.tsx?

@jungwoo3490
Copy link
Author

Could you add a unit test to packages/react-router-dom/__tests__/static-location-test.tsx?

I added it!! @brophdawg11

@brophdawg11
Copy link
Contributor

Thanks - now I think we just need a changeset and this should be good to merge. Just run pnpm changeset and it'll prompt you through the creation of one.

@jungwoo3490
Copy link
Author

Thanks - now I think we just need a changeset and this should be good to merge. Just run pnpm changeset and it'll prompt you through the creation of one.

I created changeset and commited it. @brophdawg11

@brophdawg11 brophdawg11 merged commit 127e693 into remix-run:dev May 8, 2024
3 checks passed
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

4 participants