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

Update with-redux example to match the guide in Redux Toolkit docs #59864

Merged
merged 69 commits into from
Feb 24, 2024

Conversation

aryaemami59
Copy link
Contributor

What?

Update with-redux example.

Why?

with-redux example currently:

  • does not follow the folder structure recommended by Redux Toolkit
  • does not create unique store instances per request as recommended by Redux Toolkit
  • adds the Redux Logger middleware by default which is not necessary and has type compatibility issues with Redux Toolkit v2
  • contains some outdated packages

How?

This PR:

  • Overhauls the folder structure to match the recommended folder structure
  • Creates unique store instances per request as recommended by Redux Toolkit
  • Removes the Redux Logger middleware
  • Updates outdated packages

Relevant Information

https://redux-toolkit.js.org/usage/migrating-rtk-2#nextjs-setup-guide
https://redux.js.org/usage/nextjs#folder-structure

@ijjk ijjk added the examples Issue/PR related to examples label Dec 21, 2023
@markerikson
Copy link

And for the record, I maintain Redux and Redux Toolkit, and I requested these changes to the example :)

/cc @leerob

Actual review feedback:

  • could we move those thunks from counter/thunks.ts into counter/counterSlice.ts? Don't see a reason for them to be split out
  • the README phrasing about RTK seems a bit awkward. Can we update that text for grammar, spelling, and phrasing?
  • for some reason I thought we had RTK Query set up in the Redux CRA/Vite templates, but apparently we don't. Wonder if it's worth adding that here at all, or not.

@aryaemami59 aryaemami59 marked this pull request as draft January 3, 2024 19:36
@leerob
Copy link
Member

leerob commented Jan 6, 2024

#53817 (comment)

@aryaemami59 aryaemami59 marked this pull request as ready for review January 16, 2024 18:58
@aryaemami59
Copy link
Contributor Author

@EskiMojo14 @markerikson @phryneas @leerob I made the improvements that were suggested, does this need anything else?

@EskiMojo14
Copy link

I'm happy with it :)

@leerob leerob enabled auto-merge (squash) February 24, 2024 15:01
@ijjk
Copy link
Member

ijjk commented Feb 24, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8637c48

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

1 similar comment
@ijjk
Copy link
Member

ijjk commented Feb 24, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8637c48

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@leerob leerob merged commit 7d4821d into vercel:canary Feb 24, 2024
32 checks passed
@markerikson
Copy link

Awesome, thank you!

@aryaemami59 aryaemami59 deleted the update-with-redux-example branch February 27, 2024 05:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue/PR related to examples locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants