-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix Devtools action dispatching #3333
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
|
I have to create tests now, but you can give this a look @dai-shi |
dai-shi
left a comment
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.
Thanks for working on it!
The addition of actionCreators option seems reasonable.
Not a big fan of the complexity and feels like it could be more primitive.
I have also a mixed feeling about inlining redux-devtools-utils. At least, it's hard to review and I don't think I need to review redux-devtools-utils part.
Could you create a version with using redux-devtools-utils as a peer dependency? I think we could dynamic import it, so only users who use actionCreators depend on it.
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.
Thanks for working on this! Left some comments.
Once we settled on the implementation, please add tests.
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 don't think we need to change the behavior at all for the redux middleware usage (at least in this PR). Or, I may misunderstand something.
src/middleware/devtools.ts
Outdated
| } | ||
| } | ||
|
|
||
| let usingReduxMiddleware = typeof (api as any).dispatch === 'function'; |
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.
Can this be const?
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 might have misguided you, but I think what we want here is this:
| let usingReduxMiddleware = typeof (api as any).dispatch === 'function'; | |
| const usingReduxMiddleware = (api as any).dispatchFromDevtools && typeof (api as any).dispatch === 'function' |
Sorry about my confusion.
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 dont see value in dispatchFromDevtools being present anymore as we always support dispatching from devtools now, be it having or not having redux middleware
src/middleware/devtools.ts
Outdated
|
|
||
| let usingReduxMiddleware = typeof (api as any).dispatch === 'function'; | ||
| if (!usingReduxMiddleware) { | ||
| (api as any).dispatchFromDevtools = true; |
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.
This is the flag set from the redux middleware. It means like "allow dispatching from devtools middleware".
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 think we can remove the flag if it doesnt make any difference anymore unless it's set in other cases not just with the redux middleware being present, or it carries any other meaning.
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 think you misunderstand something about the flag. It's to tell from redux middleware to devtools middleware.
Do not change that convention in this PR.
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 know it's to tell from redux middleware to devtools middleware. I'm using the flag to detect if redux middleware is being used.
Well, I was using it until I noticed it was no longer needed. I read very carefully through the code and noticed there was no single place where the flag was set to false, which would have changed things.
The only place the flag is used is for checking if the redux middleware is being used and a check in the Redux Devtools receive message subscription. Notice that if the flag stays, the check will have to be removed for this to work properly and the flag would be reduced to only informational data.
EDIT: Well, actually I am making a check to show different examples for malformed custom actions. That'd be the only valid place left.
If you want to keep the flag I would have to update the code with the changes mentioned above, let me know how you would like me to proceed. I am just explaining my reasoning on the changes, it was not a misunderstanding but careful inspection of the code. Unless it was....
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.
there was no single place where the flag was set to false, which would have changed things.
It's for the case that .dispatch is defined by a user, not by the middleware. You can't find it from the code.
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.
there was no single place where the flag was set to false, which would have changed things.
It's for the case that
.dispatchis defined by a user, not by the middleware. You can't find it from the code.
Yes I thought so, but then, what do you think would be the best approach here?
- Error if
dispatchis defined in the state? - Use a different property like
__dispatchif not using redux middleware? Tested this and work without problems, uploaded in last commit: cb6e9b9
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.
Actually, I'm not sure why we need .dispatch or __dispatch for the cases without redux middleware.
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.
Now that you mention that, I think I can inline it in the ACTION message instead. Good catch, I was just trying to use the same approach that was already used. Will try it tomorrow.
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.
Sounds good. I'll see what I can try after that.
| return; | ||
| } | ||
|
|
||
| const action = actionCreators.find((action) => action.name === payload.type); |
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.
Should we allow using actionCreators if the redux middleware is used?
I don't think we should modify dispatch.
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.
Yes, as we are trying to be compatible with Redux Devtools not adding something new. Actions creators is what lets users select options from the actions dropdown and dispatch them by filling out the properties in the form.
What this PR does is just adding support for the dropdown itself as previously one could only dispatch "Custom action" with their JS/JSON code.
Expected and current behavior is as follows:
- For custom actions, JS will always error as being malformed and show example of the right pattern depending on the type of store being used.
- If @redux-devtools/utils is not installed it will warn whenever trying to dispatch actions from the devtools. Except for custom actions on a Redux store which should always be dispatchable.
# Behavior Matrix
With @redux-devtools/utils:
Normal Store:
Custom Action:
JSON: Success
JS: Errors malformed
Action Creator: Success
Redux Store:
Custom Action:
JSON: Success
JS: Errors malformed
Action Creator: Success
Without @redux-devtools/utils:
Normal Store:
Custom Action:
JSON: Warns required
JS: Errors malformed
Action Creator: Warns required
Redux Store:
Custom Action:
JSON: Success
JS: Errors malformed
Action Creator: Warns requiredThere 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.
What this PR does is just adding support for the dropdown itself as previously one could only dispatch "Custom action" with their JS/JSON code.
Is this also for redux middleware? How can we support dropdown for it?
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.
With the redux middleware you can:
- Use the mask to expose functions in the state. (Though, I don't think this will be common in
reduxstores) - Do it the redux way and pass your actions creators to the actionsCreators option (Redux devtools docs dont have good example). It's just passing functions that return redux actions (thus the name of actionCreators).
- Create custom action creators that update the state themselves.
const useCounterStore = create()(devtools(redux((action, state) => {
switch(action.type) {
case 'increment':
return { ...state, count: state.count + (state.amount ?? 1) };
case 'decrement':
return { ...state, count: state.count - (state.amount ?? 1) };
default:
return { ...state };
}
},{
count: 0,
reset: () => {
useCounterStore.setState(useCounterStore.getInitialState());
}
}), {
actionCreators: {
// Option 1: Use mask
reset: true,
// Option 2: Pass your action creators, probably defined elsewere
increment: (amount = 1) => ({ type: 'increment', amount }),
decrement: (amount = 1) => ({ type: 'increment', amount }),
// Option 3: Mutate yourself
power: (n = 2) => {
const state = useCounterStore.getState()
useCounterStore.setState(state.count ** n);
}
}
}))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.
We don't need to support dropdown for redux middleware.
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.
with redux middleware |
Custom Action | Action Creators |
|---|---|---|
| Before | Only if JSON formatted, all actions + __setState |
Errors with "unsupported format" |
| Now | Same | Same |
without redux middleware |
Custom Action | Action Creators |
|---|---|---|
| Before | Only if JSON formatted, only __setState supported |
Errors with "unsupported format" |
| Now | Same + hint if malformed + action creators | Works as expected |
Again, I may change my mind after some reviews. Maybe, I should try forking your branch and see how it goes.
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.
if you can try the changes yourself would be great. I don't think leaving the Json parsing error as before is a good approach for custom actions. I'm okay with reverting the unsupported message for the drop down when not using redux.
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.
when not using redux.
Do you mean when using redux?
Updated:
with redux middleware |
Custom Action | Action Creators |
|---|---|---|
| Before | Only if JSON formatted, all actions + __setState |
Errors with "unsupported format" |
| Now | Nice error if it's not JSON. If it's JSON, they are actions for .dispatch or __setState |
Error mentioning to use Custom Action in JSON format |
without redux middleware |
Custom Action | Action Creators |
|---|---|---|
| Before | Only if JSON formatted, only __setState supported |
Errors with "unsupported format" |
| Now | Nice error if it's not JSON. If it's JSON, they are actions specified with actionCreators or __setState, otherwise error gracefully |
Drop down just works |
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.
Great, then we just have to confirm on the messages. the only one missing is action creators with redux middleware. The rest is already implemented
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'll wait for your refactor #3333 (comment), if you are going to do. Then, I'd jump in the code and confirm on the messages. After that, what's remaining is tests.
Related Bug Reports or Discussions
Fixes #3081
Summary
Currently, we only seem to be able to dispatch actions from the devtools when using the
reduxmiddleware along with thedevtoolsmiddleware (havent tested that case).This PR adds support for exposing the store actions so they can be dispatched through the Redux Devtools without having to create reducers or anything else.
Check List
pnpm run fixfor formatting and linting code and docs