Skip to content
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

Closed
BenLorantfy opened this issue Aug 10, 2021 · 5 comments
Closed

Provide a mergeOptions configuration to pass along to deepmerge #186

BenLorantfy opened this issue Aug 10, 2021 · 5 comments

Comments

@BenLorantfy
Copy link
Contributor

BenLorantfy commented Aug 10, 2021

Currently, it is impossible to track objects of type Error or objects that inherit from Error. This is because deepmerge treats Error 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.

@tizmagik
Copy link
Collaborator

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 merge property in the options object which can either be an object to pass in as configuration to deepmerge, or is the merge() function to use itself instead of deepmerge.

Thoughts on that approach?

@BenLorantfy
Copy link
Contributor Author

BenLorantfy commented Aug 21, 2021

@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 Error usecase via the isMergeableObject deepmerge config option, and likely enable other usecases.

If you're ok with this approach I could put a PR together

@tizmagik
Copy link
Collaborator

Yea a PR for that would be great! Only thing I’m not sure about is if we should call it merge or mergeOptions?

@BenLorantfy
Copy link
Contributor Author

I kind of like mergeOptions. Wouldn't want people to get confused and think merge should be a function

@tizmagik tizmagik changed the title Tracking Error objects Provide a mergeOptions configuration to pass along to deepmerge Feb 25, 2022
@tizmagik
Copy link
Collaborator

tizmagik commented Jun 9, 2023

This was completed:

@tizmagik tizmagik closed this as completed Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants