-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
[WIP] State management updates #345
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…efinitions for related components
Co-authored-by: Travis Lovis <[email protected]>
This update adds a series of custom alias values to the Webpack config, to match the import paths used in the tsconfig file. Together, they finally enable full support for import paths throughout the application. This update also adds some basic type-safety to the Webpack files, too, while keeping them in the .js format. Co-authored-by: Travis Lovis <[email protected]>
Co-authored-by: Travis Lovis <[email protected]>
Co-authored-by: Travis Lovis <[email protected]>
Closing pull request until discrepancies between local tests and GH Actions tests can be figured out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This update makes quite a few changes to developer ergonomics for the application. Special thanks to @tlovis for helping me out with this!
Major changes
Better type-safety for components consuming the Redux store
The Redux implementation has had a custom
useAppSelector
hook added to and exported from thestore.ts
file. It works exactly like the previoususeSelector
at runtime – there is actually zero difference in runtime functionality – but it gives you full type-safety and auto-complete support. No more risks of randomany
types or needing to guess about the structure of the store.Every single file in the application has already been converted over, so future code updates should be quite a bit less painful.
There is no more need for this:
This is fully equivalent in functionality, and is even more type-safe:
All top-level React Router components have been typed
This has certainly been one of the biggest pain points for the application. The current app architecture is still heavily based around container components and presentation components. That is,
main.tsx
, the main entry-point for the React Router setup, used Redux'sconnect
API to get every single potentially-relevant state value and dispatch, and then directly passed them down to every single Router component.While this does come with its own set of issues – the biggest one being that all state is bound to React's render cycles at all times, meaning that the app can over-render in surprising ways – it did mean that the props could be defined from one central place, and then exported for the other components to consume.
This process did highlight that not all components were consuming the values consistently, though. Because the other components went without type-checking for so long, there are some prop mismatches between components. However, there should be less of a need to blindly guess at what a given component has access to in the future. I fixed what props I could with the limited time I had for this cycle, but in the future, these prop mismatches will need to be corrected.
However, I do want to be clear: adding a centralized type is a band-aid solution. The ultimate goal should be to update each individual component to directly consume Redux via
useAppSelector
oruseAppDispatch
. These subscriptions should be "tamped down" to the lowest subtree component possible. Not only would this make it more clear where certain components are consuming values and clean up component API type, but it would also minimize which components need to re-render for any given state update.One other note: because of how React's render cycles are structured, these re-render issues are all-or-nothing. As in, until every single Route component has been fully updated, an update to one will cause an update to the others, even if those others don't receive any props. If this becomes enough of an issue, those components that don't need to re-render can opt into
React.memo
.mapStateToProps
andmapDispatchToProps
are almost completely removedAside from the
main.tsx
file mentioned earlier, every other component has been updated to use correctly-typed RTK hooks.main.tsx
is the only file left, but realistically, it probably shouldn't have any Redux hooks imported once the app-wide restructuring has been finished.Custom path aliases
Both the TSConfig and Webpack files have been updated to add support for custom import aliases, with full auto-complete support from the TS LSP.
Now instead of needing to do this:
You can import from the same absolute path for any component, no matter where you are:
All files that were consuming Redux have been updated to these imports. And because none of the files are directly importing from a specific file structure anymore, the
toolkit-refactor
folder has been renamed to justrtk
.I will note that any JS files still using
require
do not get the full benefits of this change. Using absolute paths should still work, because Webpack can still resolve the paths during the compilation step. But because these imports exist outside of TS, it can't give you auto-complete at author time.Minor changes
Webpack files have improved type-safety
Even though the Webpack files have not been converted to TypeScript just yet, the existing files have been augmented with JSDoc comments, including the
@ts-check
directive. This should give better auto-complete support when adding features to the config files, and should notify you of (some) accidental typos.Several files have been formatted
I'm hoping to make one other PR in the next few days about requiring a PR to be formatted with Prettier before it can be merged into main. In the meantime, every file that was updated from every other change has been formatted ahead of time.
One of the biggest pain points when working with PRs is figuring out what exactly changed. Having extra lines change formatting, even if they haven't changed behavior, just adds extra noise and makes it harder to focus on what really matters. As Swell continues to grow, it is absolutely vital that all contributors follow strict formatting guidelines, just to keep things manageable. For better or worse, Swell has reached a size where personal style preference cannot have a place in discussions anymore.
The first custom hook has been added
As several of the components have been using the same patterns for managing dropdown state, that functionality has been extracted out into the
useDropdownState
hook. If any more dropdowns need to be implemented in the future, please use this hook.What's next
I'm hoping to have two more PRs sent out soon. One will be in charge of enforcing style guidelines in the project, while the other will focus on developer documentation. This PR was just enough of a beast that I wanted to get this one done first, though.