fuzetsu / mergerino

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

Use a function directly instead of wrapping in SUB. #4

Closed foxdonut closed 5 years ago

foxdonut commented 5 years ago

Simplify further by assuming a function means to use it to process a value, without needing to wrap it in SUB.

For example, a patch would now be { age: age => age / 2 } instead of { age: SUB(age => age / 2) }.

This is simpler and reduces the number of imports. Now, the only need for imports is for DEL and when you want to completely replace a value with SUB.

fuzetsu commented 5 years ago

Thanks for the PR @foxdonut ❤️

I like this idea, I've considered it in the past, but was fairly satisfied with the explicitness of SUB. Considering this again though, I think it fits well into mergerino's mission to only require SUB in the rarest exception case (bypassing the recursive merge logic).

I think we've discussed this before, but can you think of an interesting way to supporting DEL through syntax? The obvious options are null or undefined, but of course there may be times where you want to include those values, maybe using SUB there as well would make sense? Thoughts?

Maybe deleting isn't actually that useful or important, so leaving it as DEL is fine, but I'd like to hear your thoughts again in light of this PR.

Example:

const state = { num: 5, prop: true }
const newState = merge(state, { num: x => x * 2, prop: undefined })
newState === { num: 10 }
foxdonut commented 5 years ago

@fuzetsu I would go with undefined for DEL, and leaving null as "setting" the property to null.

const state = { num: 5, prop: true }
const newState = merge(state, { num: x => x * 2, prop: undefined })
newState === { num: 10 }
const state = { num: 5, prop: true }
const newState = merge(state, { num: x => x * 2, prop: null })
newState === { num: 10, prop: null }

The null case is useful because sometimes you need to clean up but keep the property key present. I think undefined would work well as meaning DEL.

That would make mergerino even more awesome 💯

fuzetsu commented 5 years ago

Excellent, I agree, undefined for DEL, and null to keep key around.

You don't think this might be a bit too magical? It could easily DEL by accident if you're passing in a variable that happens to be undefined, in this case it would be a bit hard to tell that it would actually be removing the key.

let myVar
// ... bunch of logic that may or may not assing a value
update({ myVar }) // this could act as DEL in a fairly opaque way

Deleting might not be a huge deal if you were setting to undefined anyway, but for arrays it might matter more, since it would change the length 🤔

foxdonut commented 5 years ago

That's a good question, not sure if in practice it would be problematic. All I can say is that so far in my own apps things are null much more often than undefined, and to clean up I tend to set things to null, so I don't often delete keys entirely.

foxdonut commented 5 years ago

thanks @fuzetsu !