-
Notifications
You must be signed in to change notification settings - Fork 123
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
Provide a mergeOptions
configuration to pass along to deepmerge
#186
Comments
Ah yeap, good callout, and this use case makes sense. I wonder if instead of providing a flag to turn this behavior off if we should instead just accept a top-level Thoughts on that approach? |
@tizmagik That would work too. I kind of like your 1st suggestion of passing these options to deepmerge. This way consumers don't need to write or install their own merge function. This would enable the If you're ok with this approach I could put a PR together |
Yea a PR for that would be great! Only thing I’m not sure about is if we should call it |
I kind of like |
Error
objectsmergeOptions
configuration to pass along to deepmerge
This was completed: |
Currently, it is impossible to track objects of type
Error
or objects that inherit fromError
. This is becausedeepmerge
treatsError
as a mergeable object and creates a plain object with it.See: https://www.npmjs.com/package/deepmerge#ismergeableobject
and: https://github.com/nytimes/react-tracking/blob/main/src/useTrackingImpl.js#L43
This means
Error
objects will be converted to plain objects when they reach the custom dispatch function. I assume this isn't intended behaviour?Would you accept a PR that turns this behaviour off via a configuration value? This would allow the custom dispatch to handle
Error
objects. Alternatively, this could be turned off by default but that would be a breaking change.The text was updated successfully, but these errors were encountered: