es5 support for Map/Set

[runnez]

Hi guys! This is a draft solution for es5 map support. What do you think about this approach?
It should works with babel preset, also it may works in IE11 ;)

Now map draft tests passes and I'm planning to add Set support, patches, etc

[runnez]

Hmm, CI failed, I guess according this 017de43

[mweststrate]

Awesome! I'll hope to review in detail hopefully somewhere next week

[runnez]

Need to decide

• Should it work in environment without Map and Set?
  If yes, I need to add a checking
• Should it work in IE11 without babel?
  IE11 has partial support without iteration methods and difference between Map.set method and does not support new Map(iterable)

And I have doubts about a lot of copy-paste between implementations proxy and es5

[mweststrate]

1. No, as they can be polyfilled, which is the responsibility of the consumer of this package. A nice error message about the need of this would be great though.
2. I think it is fine sticking to the normal web api's, and if something doesn't work on any browser, it is up to the host to set up some polyfill. So I'd write the code as if it is targetting a modern browser.
3. I can imagine that having only an ES5 implementation might work, that is, a class based approach, and use that one in the Proxy case as well? I mean, the fact that Proxies are available, doesn't mean that we must use them :). I'd opt for code-base maintainability here.

[runnez]

No, as they can be polyfilled, which is the responsibility of the consumer of this package. A nice error message about the need of this would be great though.

We have some checking here
https://github.com/immerjs/immer/pull/437/files#diff-de99f837249e5adf62716789c7b9e80eR40
Imagine, some consumer no uses Map or Set if he updates Immer, it will fail in ES5 browsers without Map and Set

[runnez]

@mweststrate I think I've done. But I don't understand one thing. Should I update markChangesRecursively? It skips Map and Set, should skipping be explicit?

[mweststrate]

Awesome! I don't know the answer to that question from the top of my head, but if a test in which you modify an entry in a map, that is in a map, that is in an object, and that works, it's correct, as the mechanism is used to ensure that changes propagate through unmodified parents. I'm gonna try to carve out some time tomorrow to review in detail!

…

On Mon, Oct 21, 2019 at 5:49 PM Andrey Boldyrev ***@***.***> wrote: @mweststrate <https://github.com/mweststrate> I think I've done. But I don't understand one thing. Should I update markChangesRecursively? It skips Map and Set, should it be an explicit? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#437?email_source=notifications&email_token=AAN4NBAEPDGWUTRUD5VZ67DQPXMSDA5CNFSM4I5VFPDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB3AHYI#issuecomment-544605153>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBHOQ2UIXRWYHWGUNSLQPXMSDANCNFSM4I5VFPDA> .

[mweststrate]

Superficially this looks great! I'll merge it into the v4 branch, and to further local investigation there. I'll also send a maintainer invite. That will allow you to directly commit to that branch, which avoids the whole song-and-dance of maintaining your own fork :)

[runnez]

@mweststrate Glad to hear that! Thanks a lot for invitation!

[aleclarson]

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀