-
Notifications
You must be signed in to change notification settings - Fork 58
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
FEA-1258: Named redux devtools instances #797
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small things, otherwise looks good to me!
I tried locally (after applying that options
default value) with multiple stores and it worked like a charm!
@@ -193,3 +193,20 @@ class _OverReactReduxDevToolsMiddleware extends MiddlewareClass { | |||
/// ); | |||
/// ``` | |||
final MiddlewareClass overReactReduxDevToolsMiddleware = _OverReactReduxDevToolsMiddleware(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost wonder if we should deprecate this in favor of overReactReduxDevToolsMiddlewareFactory
, to encourage people to add names. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe eventually
Technically this approach of a middleware factory is not the method that the official redux-devtools uses. Their implementation looks a bit more like:
const composeEnhancers = composeWithDevTools(options);
const store = createStore(
reducer,
/* preloadedState, */ composeEnhancers(
applyMiddleware(...middleware)
// other store enhancers if any
)
);
I wasn't sure of the core reasoning behind this, so I felt a bit more comfortable "trying this out" for a bit before we consider it the de-facto way to initialize a dev tools middleware.
If you don't think that's necessary, I'm happy to deprecate and encourage the factory approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine by me 👍
Skynet test results failed initially for this build but were approved by greg.littlefield |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changes look good
- Stores and all their actions show up separately in the dev tools when
- one store is named and the other is not
- both stores are named
+10
@Workiva/release-management-p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
Motivation
As its currently implemented, we have no way to provide options to the initialization of the
overReactReduxDevToolsMiddleware
. This pr adds an additionaloverReactReduxDevToolsMiddlewareFactory
which currently only accepts aname
parameter (more can be added as needed)This directly proxies the list of available options which the redux-devtools api provides
Changes
overReactReduxDevToolsMiddlewareFactory
to allow constructing a dev tools middleware with a name parameterRelease Notes
Add
overReactReduxDevToolsMiddlewareFactory
which allows constructing redux dev tools with optionsReview
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: