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

Map & Set support (v5) #354

Merged
merged 48 commits into from
Oct 30, 2019
Merged

Map & Set support (v5) #354

merged 48 commits into from
Oct 30, 2019

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Apr 18, 2019

Try v5.0.0 today!

npm install immer@next

Tasks

  • ES6 Map / Set support with setUseProxies(true)
  • Ensure onDelete/ onAssign hooks work with ES6 maps/sets (and write tests)
  • Ensure object keys can be used with ES6 maps
  • ES6 Map / Set support in ES5 mode
  • Update typings for ES6 maps/sets
  • Update docs website
  • fix build
  • process review comments, create follow up tasks, also for typings
  • verify build size: 4.59 -> 6.28 gzipped, minified
  • check for duplicate code in traps, source fn
  • check for unnecessary closures in traps etc
  • how to freeze map / set?
  • check perf impact -> no significant changes

@aleclarson aleclarson mentioned this pull request Apr 18, 2019
@mweststrate
Copy link
Collaborator

Might be nice to have the additional tests from #353 in this branch, in case we missed something?

@aigoncharov
Copy link

aigoncharov commented Apr 18, 2019

@mweststrate I wish you had told me you wouldn't let the PR for ES6 only maps to pass through in that thread. Though, yeah, it all happened really fast :)
P.S. I have a working POC for es6 Sets. Will raise a PR with it for this branch soon.

@aigoncharov
Copy link

aigoncharov commented Apr 18, 2019

Do we want to have ES5 support or do we want to have IE11 support? I believe it's not the same thing. IE11 doesn't support iterators and lots of other methods according to MDN.
This would still require to run some tests conditionally. It still would be an issue if a project, successfully using Maps, all of a sudden has to start supporting IE11. That kind of project still would have to be refactored.
@mweststrate are you firm on not letting ES6 only implementation pass through? Since it's gonna be a major release anyway, I guess it's ok to break things. Though IE11 folks wouldn't even be broken, they'll just have to stick to the old way of working with Maps manually. What we could do in this case is to add tests that the old way still works as expected.
P.S. New MobX works only in Proxy-enabled environments, which makes me wondering if new Immer should follow the same path.

@mweststrate
Copy link
Collaborator

mweststrate commented Apr 22, 2019 via email

@aleclarson aleclarson force-pushed the v4 branch 2 times, most recently from 5e6dcf8 to c90cd57 Compare April 23, 2019 01:10
@aleclarson
Copy link
Member Author

Added patch-related functionality for ES6 Sets.

Items to discuss:

  1. Patches for Maps will work only for string and numeric keys. Is it ok? Should we add a section to README only or have an exception thrown?
  2. I'm not sure how compatible with RFC-6902 JSON patch standard is this record for ES6 Set patches. Currently, these patches cannot be applied in a different order.
  3. Do we really-really-really want to support ES5? :)

TODOs:

  • Typings
  • Add tests (and most probably add missing implementation) for onDelete and onAssign hooks for cases with ES6 Maps and ES6 Sets

@aleclarson if you merge it to v4 branch, I'd be happy to carry on this discussion there, since it really does belong there.

Originally posted by @keenondrums in #358 (comment)

@aleclarson
Copy link
Member Author

  1. Patches for Maps will work only for string and numeric keys. Is it ok? Should we add a section to README only or have an exception thrown?

No, we should make sure changes to object keys have patches created for them, since Immer patches can be useful even when not serialized to JSON.

Note: Immer prefers working with tree-like data structures, and I'm not sure what the bugs are when relational data is used.

2. I'm not sure how compatible with RFC-6902 JSON patch standard is this record for ES6 Set patches. Currently, these patches cannot be applied in a different order.

To be clear, Immer patches aren't strictly JSON patches, but you must use JSON-only keys/values if you ever need to serialize the patches.

3. Do we really-really-really want to support ES5? :)

To not support ES5 would add unwanted complexity to the API for ES5 users. :(

Originally posted by @aleclarson in #358 (comment)

@aigoncharov
Copy link

aigoncharov commented Apr 23, 2019

@aleclarson do we want to support ES5 (read it as environments that lack Proxy support) or IE11? It's not the same thing. As MDN states IE11 doesn't support new Map([iterable]) which means we'll have to come up with a different mechanism to shallow copy Maps in IE11.
Moreover, we do not run tests specifically for IE 11 using that very IE11. It means we can not really guarantee we support it.

@mweststrate
Copy link
Collaborator

Some random thoughts:

I think it is fine to limit maps to only allow strings / numbers as keys. I think having objects as keys, which can then be updated in a producer as well, will get confusing very quickly, and just bailing out is probably better until we encounter a good RL use case

The goal is to make sure devs can use immer safely inside consumer facing applications, without sudden suprises. In practice that means that we need to support ES5, or at least, things that can be polyfilled to ES 5. So in this specific case, whether Map([iterable]) isn't that important, the important question is, does the implementation correctly after being processed by babel? If babel polyfills the map constructor correctly, it is fine. In MobX there are two major versions still being maintained, which is also not ideal. But at least, practically all new functionality in 5 can be expressed in mobx 4, without big semantic changes.

The risky semantic difference here is that sets and maps would be treated totally different as soon as they are considered draftable. So it could be an option to only support sets and maps in a next major, but I think it would be a significant maintenance burden. Mostly being helping people out that, ignorantly, npm installed the latest version, work a while with that, use maps / sets (probably without checking the docs on that at all seemed to works ootb), and then discover that they have to rewrite a lot of code after a first round of QA on IE11...

@omninonsense
Copy link

omninonsense commented May 1, 2019

  1. Patches for Maps will work only for string and numeric keys. Is it ok? Should we add a section to README only or have an exception thrown?

I don't think there's much valid use-cases outside of strings/numbers anyway. I think the main benefit of using a Map over an object is convenience (ie numbers not being coerced into strings when the keys are numeric IDs, easier/safer preservation of type information in Flow/TS).

@shabbyrobe
Copy link

I don't think there's much valid use-cases outside of strings/numbers anyway.

It's actually a very important use-case for Sets and Maps. Most of the time I reach for them it's for this property.

@aigoncharov
Copy link

@shabbyrobe I have to correct my statement. I think any valid key (including objects, symbols and etc.) is going to work with Maps. I just doubt if it's going to work with patches enabled.

@mweststrate
Copy link
Collaborator

I resolved the merge conflicts. There might be some mistakes in there, as it was quite tricky. Some tests related to patches are still failing at least, most notable is this error (does it ring a bell @runnez)?

TypeError: Method Map.prototype.forEach called on incompatible receiver #<Map>
        at Map.forEach (<anonymous>)

      113 | export function each(obj, iter) {
      114 | 	if (Array.isArray(obj) || isMap(obj) || isSet(obj)) {
    > 115 | 		obj.forEach((entry, index) => iter(index, entry, obj))
          | 		    ^
      116 | 	} else {
      117 | 		ownKeys(obj).forEach(key => iter(key, obj[key], obj))
      118 | 	}

      at forEach (src/common.js:115:7)

Coming days I won't have time to further investigate, so I'll continue after that, or if anyone beats me to it, welcome :).

Sorry, this got a lot more complicated than it ought to be as there were significant changes on master as well, but it is tricky to find them one by one due to the whitespace changes.

I think it should be possible to build the traps up a bit more efficiently, but let's get the tests passing first.

@aleclarson
Copy link
Member Author

aleclarson commented Oct 23, 2019

Sorry, this got a lot more complicated than it ought to be as there were significant changes on master as well, but it is tricky to find them one by one due to the whitespace changes.

@mweststrate Git has options for merging without whitespace conflicts (see here). Maybe you should rebase and keep the tabs in place?

@mweststrate
Copy link
Collaborator

mweststrate commented Oct 23, 2019 via email

@runnez
Copy link
Collaborator

runnez commented Oct 23, 2019

@mweststrate I've found a place where logic is broken. If we move back an old function, it will fail but only one test. I'll try to fix it tomorrow

export function applyPatches(draft, patches) {

  ● applyPatches › applied patches cannot be modified

    TypeError: Cannot add/remove sealed array elements
        at Array.splice (<anonymous>)

      154 | 			const add = (key, value) =>
      155 | 				Array.isArray(base)
    > 156 | 					? base.splice(key, 0, value)
          | 					       ^
      157 | 					: isMap(base)
      158 | 					? base.set(key, value)
      159 | 					: isSet(base)

      at splice (src/patches.js:156:13)
      at add (src/patches.js:176:6)
      at src/immer.js:166:4
      at Immer.recipe (src/immer.js:74:14)
      at Immer.produce [as applyPatches] (src/immer.js:165:15)
      at Object.<anonymous> (__tests__/patch.js:105:3)

 PASS  __tests__/base.js

Test Suites: 1 failed, 10 passed, 11 total
Tests:       1 failed, 1730 passed, 1731 total
Snapshots:   143 passed, 143 total
Time:        4.014s

@runnez
Copy link
Collaborator

runnez commented Oct 25, 2019

@mweststrate I've fixed tests, but I've got doubts about expected behaviour on this line ff25a83#diff-b40ab5eb20dfada012e789db3e9780bbR142

@runnez
Copy link
Collaborator

runnez commented Oct 25, 2019

@mweststrate I've fixed errors in ie11, but you may revert it if need ffd8769

src/proxy.js Outdated Show resolved Hide resolved
src/proxy.js Outdated Show resolved Hide resolved
@mweststrate
Copy link
Collaborator

@runnez thanks for investigating! Sorry for messing up that if-else logic while merging 😅

@mweststrate
Copy link
Collaborator

mweststrate commented Oct 28, 2019 via email

@runnez
Copy link
Collaborator

runnez commented Oct 28, 2019

I think the condition is incorrect; even if the base is a set, and the op is add, the patch should be defended against accidental mutation, as theoretically a next patch could change that value inside that set (and otherwise, if we are going to error anyway, it doesn't matter that we didn't optimize this clone anymore)

@mweststrate did I understand you correctly? 24658bc

Copy link
Collaborator

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runnez cool, thanks for the changes, that was what I was looking for indeed!

Did an in-depth review / test, and I think this is starting to look really solid now! Left a bunch of comments, but I think all the hard stuff is done :) Just pushed a few commits as well, so feel free to review them.

obj2.val = "you"
},
[
{op: "remove", path: [0], value: {id: 1, val: "We"}},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect patches here that capture that the modification was only the object inside the sets, rather than patches on the set itself, like:
op: "replace", path: [1, "val"], value: "will"

Is there a special reason the entire set entry gets reconstructed? (I agree that the concept of indices in a set can be challenged, and maybe your approach is more correct, but sets guarantee insertion order, so I think it should be ok rely on paths in sets?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this quite an edge case anyway, so we could also open it as a fresh follow up issue, and see if anybody runs into the difference, before doing anything.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ambiguity of indices in Sets was the primary reason. As you said, you can iterate over them, however, you cannot get an item at index.
Love the idea of leaving this edge case out of the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #450

src/es5.js Show resolved Hide resolved
* Map drafts
*/

const mapTraps = makeTrapsForGetters({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random thought: could we use the ES5 implementation, and not use Proxies for maps and sets at all? If that would simplify the implementation that would be cool. If "ES5" administration doesn't play nicely inside a proxy draft tree, than lets leave it as is. But proxies themself don't add much for maps and sets, since all object interactions are through functions anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also be opened as follow up optimization issue, to reduce the package size

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible to use one codebase for Map and Set and use both Proxy and ES5 mechanics. Let's do it as a follow up issue

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #449

@mweststrate
Copy link
Collaborator

mweststrate commented Oct 30, 2019 via email

@mweststrate mweststrate merged commit b6f1d40 into master Oct 30, 2019
@aleclarson
Copy link
Member Author

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants