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.

Sign up for GitHub

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

Jump to bottom

Consider enabling default middleware checks on Redux store #1712

Merged
merged 4 commits into from
Oct 17, 2023
Merged

Consider enabling default middleware checks on Redux store #1712

merged 4 commits into from
Oct 17, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Oct 12, 2023 •
edited
Loading

Follow-up to dcd83c0.

Checklist

  • Checks pass
  • (to be done by a Nextstrain team member) Create preview PRs on downstream repositories. N/A, no functional changes
  • (local test) Setting ignoredPaths to an empty array and running export SKIP_REDUX_CHECKS=true && npm run dev shows no console warnings/errors related to the immutability middleware.

@victorlin victorlin self-assigned this Oct 12, 2023
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-enabl-lumbpn October 12, 2023 22:32 Inactive
victorlin
victorlin commented Oct 12, 2023
View reviewed changes
src/store.ts Outdated Show resolved Hide resolved
@victorlin victorlin mentioned this pull request Oct 12, 2023
Merged
3 tasks
@victorlin victorlin force-pushed the victorlin/enable-redux-store-middleware-checks branch from e2e2138 to b0199d6 Compare October 13, 2023 23:58
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-enabl-lumbpn October 13, 2023 23:59 Inactive
@victorlin victorlin marked this pull request as ready for review October 14, 2023 00:02
@victorlin
Add context for getDefaultMiddleware
de9f63d 
The function name itself isn't very informative. Add more info from the
doc page¹.

¹ https://redux-toolkit.js.org/api/getDefaultMiddleware#included-default-middleware
@victorlin
Keep serializability check disabled
0a2792d 
Reasoning added as a comment.
@victorlin
Partially enable immutability check
2941948 
This is useful but makes things slower upon state changes. I think still
reasonable to have because:

1. It's only included in dev mode
2. The slowness in dev mode is not too noticeable (50~200ms on my machine)

The console warnings hint to disable it if the slowness becomes
unbearable.
@victorlin victorlin force-pushed the victorlin/enable-redux-store-middleware-checks branch from b0199d6 to 2941948 Compare October 14, 2023 00:05
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-enabl-lumbpn October 14, 2023 00:05 Inactive
@victorlin victorlin changed the title Enable immutability and serializability checks on Redux store Reconsider enabling default middleware checks on Redux store Oct 14, 2023
@victorlin victorlin changed the title Reconsider enabling default middleware checks on Redux store Consider enabling default middleware checks on Redux store Oct 14, 2023
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-enabl-lumbpn October 16, 2023 18:17 Inactive
jameshadfield
jameshadfield reviewed Oct 17, 2023
View reviewed changes
README.md Outdated
@@ -75,6 +75,18 @@ If you are editing source code, running the following command will allow hot-rel
auspice develop --datasetDir data
```

#### Environment variables

The server looks for some environment variables. All are optional.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but /server/client/. (It's been a perpetual source of confusion to people over the years as Auspice can mean both the server and the client.)

The server does also look for some variables, the most common being PORT and HOST, so over time we can include them.

And then there's variables which are essentially compiler arguments, e.g. BABEL_*, which I would categorise separately to the server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the clarification. This is something I haven't looked into much and doesn't seem to be spelled out in docs. I'll look further and update docs in a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, I see it's in the RTD docs. I'd think to move all dev-related docs there since currently those are split between README, DEV_DOCS, and RTD docs.

jameshadfield
jameshadfield approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Looks good, pending the one minor syntax change.

@victorlin victorlin force-pushed the victorlin/enable-redux-store-middleware-checks branch from 7d14334 to a003a32 Compare October 17, 2023 16:59
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-enabl-lumbpn October 17, 2023 17:00 Inactive
@victorlin victorlin merged commit 2e5792b into master Oct 17, 2023
19 checks passed
@victorlin victorlin deleted the victorlin/enable-redux-store-middleware-checks branch October 17, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameshadfield jameshadfield jameshadfield approved these changes

Assignees

@victorlin victorlin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@victorlin @jameshadfield @nextstrain-bot