-
Notifications
You must be signed in to change notification settings - Fork 119
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
Infinite depth configuration #165
Infinite depth configuration #165
Conversation
Hi Jason @GravlLift, Thanks so much for the PR! This is great. I love how you've cleaned up the code and added types. Can you please:
Once you've made those changes I'll have another look. |
All set @BBlackwo.
|
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 addressing all of those things! I've just got a few more minor comments if you can have a look please.
The only one that needs to be addressed is the deserialize
function missing from the type. All the rest are optional.
You may want to squash the fixes together too.
Once that's addressed I'll merge it in and it will be automatically released as the next minor version.
Updated according to comments. On an unrelated note, I'm not sure exactly how the commit/release note tool works, but it might be useful to apply that commit formatting restriction only to the master branch, and then simply squash PRs into a single commit with the correct formatting. That way you can have messy PR branches that you can update and commit as much as you like without restriction, but that mess gets condensed and removed into a single commit with a useful message when it goes to master. I'm rewriting the same commit over and over in this branch with force push anyways, so in essence you're losing that branch history regardless. Just a suggestion. |
Thanks for fixing those up 👌
That's a good idea. It looks like semantic release might not work well with squashed commits, sadly. But I'll keep it in mind! |
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 so much for this enhancement, Jason @GravlLift!
🎉 This PR is included in version 10.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sorry it took so long for me to merge this in, btw. Appreciate your patience and continuing to address my comments months later. |
This PR allows for more targeted configuration of state, removing the restriction that only top-level data can be stored. For instance, this is now a valid configuration:
The above configuration would result in a localstorage value under the key
account
that looked like so, ignoring all other values in account, settings, or settingsGroup1:Upgraded TS to 3.9.7
Added .editorconfig