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(atomWithHash): replaceState function to use window.history.state instead of null #24

Merged
merged 2 commits into from
Oct 15, 2023

Conversation

joetis06
Copy link
Contributor

@joetis06 joetis06 commented Oct 14, 2023

My project uses the new NextJS app router, and we were using atomWithHash to keep track of state. This was breaking when used with the app router when using the back button in the browser. In my project I am just setting the setHash function to do what I committed here. I figured it would be better and easier for anyone else using this library to just set the flag replaceState if they are using the nextjs app router. (Maybe the website/documentation should be updated as well.)

… instead of null

My project uses the new NextJS app router, and we were using atomWithHash to keep track of state. This was breaking when used with the app router when using the back button in the browser. In my project I am just setting the setHash function to do what I committed here. I figured it would be better and easier for anyone else using this library to just set the flag replaceState if they are using the nextjs app router. (Maybe the website/documentation should be updated as well.)
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 14, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fce6862:

Sandbox Source
React Configuration
React TypeScript Configuration

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

The code looks good. I don't think we can add a test for it, right?

null,
window.history.state,
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch. I didn't realize this would be better.
ref: https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState

@dai-shi dai-shi changed the title Update atomWithHash replaceState function to use window.history.state… fix(atomWithHash): replaceState function to use window.history.state instead of null Oct 14, 2023
@dai-shi
Copy link
Member

dai-shi commented Oct 15, 2023

(Maybe the website/documentation should be updated as well.)

Please send PRs too!

@dai-shi dai-shi merged commit 7190eff into jotaijs:main Oct 15, 2023
1 check passed
@joetis06
Copy link
Contributor Author

The code looks good. I don't think we can add a test for it, right?

Not to my knowledge.

@joetis06
Copy link
Contributor Author

(Maybe the website/documentation should be updated as well.)

Please send PRs too!

pmndrs/jotai#2183

@dai-shi
Copy link
Member

dai-shi commented Nov 9, 2023

Hi, I just realized that we should make the same change for atomWithLocation.
@joetis06 Would you like to work on it please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants