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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip key in shallowCopy #392

Closed
zewa666 opened this issue Jun 26, 2019 · 20 comments

Comments

@zewa666
Copy link
Contributor

commented Jun 26, 2019

馃殌 Feature Proposal

It would be great if the user could control the shallowCopy in a way to tell what keys to skip during the process. The easiest thing would be if we could provide a predicate which returns bool at this specific part providing it the key and base.

Motivation

One of the possible use-cases for this might be for Frameworks such as Aurelia and it's state management plugin Aurelia Store to by-pass the built in observable trackers placed on objects by Aurelia.

Example

I've no idea how to register the predicate in order to consume it in the commons helper but maybe something along these lines:

import { setSkipCopyPredicate } from "immer";

function myPredicate(base, key) {
  return (key !== "FOOBAR")
}

setSkipCopyPredicate(myPredicate);

@zewa666 zewa666 added the proposal label Jun 26, 2019

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

Help me understand the issue better. Why would you want to bypass the "observable trackers" added by Aurelia? Please provide some example code as well.

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

the issue with those is that they are computed getter/setters, and out of the box throw an error with Immer.js which pretty much makes Immer unusable with Aurelia. Same as it seems with Vue and Ember as described here

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@aleclarson, is there anything else I can do?

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

Are you sure you want to skip computed properties? That seems like it would break the observability that Aurelia provides, but that might make sense depending on your use case (I've never used Aurelia). Could you setup a CodeSandbox that demonstrates how you're using Immer with Aurelia?

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

This is not about Aurelia itself but its state management plugin called Aurelia Store. So its for a pretty narrowed down use case where people are still able to use alternative ways like Object.assign spreads and so on.
Moreover we'll be looking into means to address this in future with a core solution. But for now the current behavior of throwing errors on computeds makes Immer pretty much unusable for Aurelia and so it seems also other frameworks.

There might also be other use cases, such as strippin computeds altogether if the resulting object e.g is going to be serialized in the end (localstorage, Redux devtools, ...)

Heres a repro link (Open the devtools console) https://gh3so.codesandbox.io/

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

If the computed properties are tracked, it sounds like they can change over time. In other words, that you are dealing with a mutable concept, which seems to be a conceptual mismatch with Immer in the first place? Conceptually, Immer 'clones' objects that have been mutated. However, there is no "correct" way to clone computed properties (Object.assign / spread operator also simply read and copy over the value).

Also read this thread for some background: #317 (comment)

For me, this sounds like you want to hack Immer, so that a library that hacks around Aurelia keeps working :). Not sure how deep you want the hacks to go, but I would recommend to skip immer in this case, and do the object cloning manual / or with your own purpose built utilities, so that at least you can control and comment explicitly your own code what the library limitations are with Aurelia (store) that are you are working around.

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

You've pretty much summed it up very well. Immer indeed doesnt fully fit how Aurelia works out of the box with Change Tracking, where nothing is wrong with that just two different concepts.

I would not per se call it a hack but an educated descision to ditch performance for a different API. I agree that I personally would restrain from using Immer with the Store plugin but that doesnt mean its not ok for somebody else. The long term solution would indeed be to properly handle aurelias getters but I doubt thats a real fit for immer as you'd have to add specific cases for a lot frameworks then. So this proposal was a solution without too many interference in Immers source.

@ctrlplusb

This comment has been minimized.

Copy link

commented Jun 29, 2019

Hey @zewa666 - interesting enough I had the same requirement for a state management lib that I have been authoring. It's an abstraction of redux that uses immer at the reducer level, however I introduced a computed property API backed by getters.

I ended up creating a fork of immer - immer-peasy. Instead of throwing an error when attempting to access a getter property my fork simply assigns the resolved value against the clone that immer creates. The value of the computed properties are then available for logic within the reducer. After the reducers have completed I rebind the getter properties.

I am going to be looking at trying to clone the actual getter via the descriptor but I am not sure if this is possible as of yet. This would then allow me to avoid the rebind logic.

Beauty of open source I guess. 馃榾

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@aleclarson wondering, could the onCopy hook be used to customize this behavior? or is that too late?

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Too late. The onCopy hook is called after each subtree is finalized.

Here are the lines that call shallowCopy, which throws the computed property error:

immer/src/proxy.js

Lines 176 to 183 in 2f911b4

function markChanged(state) {
if (!state.modified) {
state.modified = true
state.copy = assign(shallowCopy(state.base), state.drafts)
state.drafts = null
if (state.parent) markChanged(state.parent)
}
}

immer/src/es5.js

Lines 117 to 126 in 2f911b4

function clonePotentialDraft(base) {
const state = base && base[DRAFT_STATE]
if (state) {
state.finalizing = true
const draft = shallowCopy(state.draft, true)
state.finalizing = false
return draft
}
return shallowCopy(base)
}

immer/src/immer.js

Lines 223 to 231 in 2f911b4

finalizeTree(root, rootPath, scope) {
const state = root[DRAFT_STATE]
if (state) {
if (!this.useProxies) {
// Create the final copy, with added keys and without deleted keys.
state.copy = shallowCopy(state.draft, true)
}
root = state.copy
}

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

How about adding a symbol to individual objects that defines how to handle getters?

import {COPY_GETTERS, UNWRAP_GETTERS} from 'immer'

const foo = new Foo()
Foo.prototype[COPY_GETTERS] = true

const bar = new Bar()
Bar.prototype[UNWRAP_GETTERS] = true
@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Referring back to the original proposal, is there anything specifically speaking against that approach with a registered callback. I still think that offers the best flexibility.

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Yeah, the problem I have with it is that it is quite an open ended feature; it solves only one very specific use case, but not other ones brought up earlier (such as with redux persist). It only supports skipping, but has no ability to read and copy, or to reuse the descriptor etc.

Also it is quite intrusive (some attributes called foobar on completely different objects might actually be valuable, and someone not aware of the callback that was setup, will be puzzled for a long time why immer suddenly behaves completely differently for a very specific attribute).

Finally, since that callback would need to be run on each and every attribute, it can become quite expensive.

So to me it feels like patch on feature that service a too specific use case only, and just leaves a precedent for 100-a-bit-similar-but-not-exactly-the same feature request and api expansions.

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Ok, fair argument. I dont necessarily agree with your view since I was asking for indeed a very specific use case of controlled skipping but I do get kinda your reasoning for not wanting to introduce more complexity.

Just on a different idea, what about allowing to switch out the whole shallowCopy function? That would widen the use cases and at the same time by keeping the current one as default, not introducing any additional computational burden.

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@zewa666

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Yeah I know about patch package, which indeed is a great tool. If that would have been the way there wouldnt have been this issue ;)

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

I think the least obtrusive solution would be as suggested here, but I don't think generally speaking that there are enough champions for that feature to justify the added complexity, compared to the amount of current users. So closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.