fuzetsu / mergerino

immutable merge util for state management
MIT License
71 stars 6 forks source link

Can't use HTML elements as values #12

Open CreaturesInUnitards opened 2 years ago

CreaturesInUnitards commented 2 years ago

merge({}, {el: document.body}) returns { el: {} }

fuzetsu commented 2 years ago

👋 Hey Scotty, been a while!

mergerino treats document.body as an object and tries to recursively loop through it to merge it, but document.body doesn't expose its keys so this results in an empty object.

Object.keys(document.body) == []

The easy workaround would be to bypass the merge and just replace the value at that key using a function:

merge({}, {el: () => document.body}) == {el: document.body}

At the very least this should be documented with the workaround, but it's worth considering how it could be handled more nicely.

Maybe a specific check could be added to treat HTML elements as straight assignments rather than trying to merge them, but having a specific check for just HTML elements could be a bit inefficient/odd.

Another idea could be to only attempt to merge objects where the .constructor === Object, that would exclude anything other than POJOS from being deep merged..

That could be a bit restrictive/opinionated though, it would mean class instances wouldn't get merged in... Then again that might kind of make sense 🤔

Would have to consider performance as well, not sure how cheap .constructor is to check.

CreaturesInUnitards commented 2 years ago

👋 Hey Daniel!

Yeah the function patch is fine for me, just thought I'd point out the undocumented/unexpected with non-POJOs. FWIW in my non-scientific test the perf hit for this:

else if (val === null || typeof val !== 'object' || val.constructor !== Object || Array.isArray(val)) copy[k] = val

is significant, percentage-wise, but not really meaningful for RW cases, I wouldn't think.