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.
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
Field extensions #264
Field extensions #264
Changes from all commits
c5582fc
447d088
2312d43
63f2551
b42440f
0261d1d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 we should only show this warning if we detect that the state contains one of redux-undo's field names, or an array/primitive?
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 thought about that. The issue is you would be checking the returned object for every action that was dispatched to the wrapped reducer (there is no way to check only the first time and then trust it from then on).
I thought it would be best to warn the user exactly once at initialization, no matter what the user has in their state. It should also be very clear in the documentation that this is the case (I can make it more explicit).
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.
yeah, I see, that makes sense. then let's keep it as is but put the
disableWarnings
option as an argument to the function. let's also separate the field extensions into a separate package.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 👌
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.
also here, should we only warn when we detect that it is overriding something?
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.
Same idea as above.
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.
document these in README as well?
do you think the field extensions should be part of redux-undo core, or maybe a separate library? I don't think everyone will need it and it slightly increases the bundle size
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 had briefly mentioned in the PR description that these were WIP and would come out soon, but I realize that it is silly to leave them there. I'll remove them until they are fully implemented. 👍
As for separating them into a different package, that sounds like a good idea! They are purposely optional and, yes, not everyone needs it in their bundle (though tree-shaking is starting to mitigate the problem).