renameAll deleting keys

[jesspoemape]

For #38

Collaborator: @rpearce

[rpearce]

One less thing to polyfill, IMO, but I don't think we're willing to die on this object copying hill

[mgreystone]

Why are we iterating twice anyway? Can we get by with iterating over obj once?

I just did this elsewhere. Is just for rename, but same principle: https://github.com/articulate/authentic/pull/21/files#diff-27a1c86133de4310373decfdad200a91R1

I think is may also solve this problem if you don't have to track state of what we've already touched.

const newObj = {}
for (let prevKey in obj) {
  const nextKey = renames[prevKey] || prevKey
  nextObj[nextKey] = obj[prevKey]
}

[rpearce]

It keeps it as close to the original implementation as possible, but if this is a better idea, then ✅ . @jesspoemape want to give it a go?

[mgreystone]

I re-read my comment, & i think i came off stronger than i intended. Apologies.

Nevertheless, if we can resolve the problem by removing complexity instead of adding, i think feel we should. Unless there's a detail i'm missing somewhere?

[rpearce]

(This is that hill I was talking about not being willing to die on.)

Short of refactoring how you suggest (up to Jessica if she wants to), the alternative would be to take out the loop and put back in the Object.assign so that the only added complexity is the conditional to determine "do you delete the key or not?"

[jesspoemape]

@rpearce @mgreystone
Just pushed up a refactor as per Markus' suggestion

[rpearce]

That's pretty slick

[spencerfdavis]

🌚

[evilsoft]

Are we 💯 this is not the expected behavior? This seems like a Major breaking change. JIC people relied on this behavior to avoid an additional pick or omit.

I would think that with a name like renameAll it is expected that all keys should be provided.
Would it make more sense to create a new function called renameSome?

[rpearce]

@evilsoft the current implementation will either drop or keep a key based on where that key shows up in the renaming object, and this feels like a super accidental bug to me. The scenario is when you want to rename A to B and C to A -- the current implementation will delete A entirely if it is in the wrong order inside the renaming object.

@jesspoemape did I frame this correctly?

[evilsoft]

@rpearce ah, yes, good point I see that now.
Makes sense!