Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat: Add ES6 Sets support #358
Conversation
Includes support for: - mutable drafts in `produce` callbacks - patch listeners BREAKING CHANGE: `Map` objects are now drafted
|
This one doesn't have patch-related functionality yet. Will work on it as soon as I have some free time. However, I believe we could merge it in and have one extra PR with patches for Sets opened later. |
|
Nice work! |
| : Reflect.get(state, prop, receiver) | ||
| }, | ||
| ownKeys(state) { | ||
| return Reflect.ownKeys(source(state)) |
aleclarson
Apr 22, 2019
Collaborator
This trap can be removed
This trap can be removed
aigoncharov
Apr 22, 2019
Author
Contributor
Wouldn't it return keys of our state then?
Wouldn't it return keys of our state then?
aleclarson
Apr 22, 2019
Collaborator
Good catch. 👍 I remember adding this trap to fix an internal issue, then removing it once realizing it's not necessary to fix the internals. I didn't think of users calling Object.keys on their Set/Map objects, which definitely shouldn't return the keys of the draft-state.
Good catch. Object.keys on their Set/Map objects, which definitely shouldn't return the keys of the draft-state.
aleclarson
Apr 22, 2019
Collaborator
If we care about the Object.keys edge case, we should also support Object.getOwnPropertyDescriptor by adding a getOwnPropertyDescriptor trap.
If we care about the Object.keys edge case, we should also support Object.getOwnPropertyDescriptor by adding a getOwnPropertyDescriptor trap.
aleclarson
Apr 22, 2019
Collaborator
And what about the getPrototypeOf or setPrototypeOf traps? And the defineProperty, set, deleteProperty traps? It gets to be a bit much, but for the sake of correctness, we should probably do it?
And what about the getPrototypeOf or setPrototypeOf traps? And the defineProperty, set, deleteProperty traps? It gets to be a bit much, but for the sake of correctness, we should probably do it?
aigoncharov
Apr 22, 2019
Author
Contributor
I guess we're gonna need tests to cover them as well.
I guess we're gonna need tests to cover them as well.
|
@aleclarson addressed your comments: either made a corresponding change or replied in a thread. |
|
Added patch-related functionality for ES6 Sets. Items to discuss:
TODOs:
@aleclarson if you merge it to v4 branch, I'd be happy to carry on this discussion there, since it really does belong there. |
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.
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.
To not support ES5 would add unwanted complexity to the API for ES5 users. :( |
aleclarson
force-pushed the
immerjs:v4
branch
2 times, most recently
from
5e6dcf8
to
c90cd57
|
Merged into the |
No description provided.