Skip key in shallowCopy

[zewa666]

🚀 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);

[aleclarson]

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]

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]

@aleclarson, is there anything else I can do?

[aleclarson]

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 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]

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: https://github.com/immerjs/immer/issues/317#issuecomment-

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]

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]

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]

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

[aleclarson]

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:

https://github.com/immerjs/immer/blob/2f911b4c29049e42193142eac3d73821fb288137/src/proxy.js#L176-L183 https://github.com/immerjs/immer/blob/2f911b4c29049e42193142eac3d73821fb288137/src/es5.js#L117-L126 https://github.com/immerjs/immer/blob/2f911b4c29049e42193142eac3d73821fb288137/src/immer.js#L223-L231

[mweststrate]

Maybe we could introduce an option to define getter / setter treatment: "getterHandling": "error" (default) / "copy-value" / "copy-descriptor". However, I can imagine it doesn't compose really nicely, as the setting would basically global.

On Thu, Jul 4, 2019 at 5:25 PM Alec Larson notifications@github.com wrote:

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:

https://github.com/immerjs/immer/blob/2f911b4c29049e42193142eac3d73821fb288137/src/proxy.js#L176-L183

https://github.com/immerjs/immer/blob/2f911b4c29049e42193142eac3d73821fb288137/src/es5.js#L117-L126

https://github.com/immerjs/immer/blob/2f911b4c29049e42193142eac3d73821fb288137/src/immer.js#L223-L231

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/immerjs/immer/issues/392?email_source=notifications&email_token=AAN4NBALGYZBGNOP333FUALP5YI7FA5CNFSM4H3PDLNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZHWBQY#issuecomment-508518595, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBG5QN5PKWCPJERKBJLP5YI7FANCNFSM4H3PDLNA .

[aleclarson]

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]

Dunno, get's a bit messy, especially for things that don't have a prototype, it means that you would need to instrument that on all instances.

On Thu, Jul 4, 2019 at 5:37 PM Alec Larson notifications@github.com wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/immerjs/immer/issues/392?email_source=notifications&email_token=AAN4NBBIEVKZATHL3UKAI33P5YKKZA5CNFSM4H3PDLNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZHW2CA#issuecomment-508521736, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBEV2I7XGNEZ67XXW4DP5YKKZANCNFSM4H3PDLNA .

[mweststrate]

But, not fully convinced we need the feature at all TBH :P

On Thu, Jul 4, 2019 at 5:51 PM Michel Weststrate mweststrate@gmail.com wrote:

Dunno, get's a bit messy, especially for things that don't have a prototype, it means that you would need to instrument that on all instances.

On Thu, Jul 4, 2019 at 5:37 PM Alec Larson notifications@github.com wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/immerjs/immer/issues/392?email_source=notifications&email_token=AAN4NBBIEVKZATHL3UKAI33P5YKKZA5CNFSM4H3PDLNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZHW2CA#issuecomment-508521736, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBEV2I7XGNEZ67XXW4DP5YKKZANCNFSM4H3PDLNA .

[zewa666]

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]

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]

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]

Just for verification, the earlier proposed setting for how getters should be treated also fixes it, correct? (well, if it also has an "skip" option, forgot about that one, sorry!)

Configuring the copy function might be hard, because it might be significantly more complex in the next major, to support maps and sets (IIRC). Note that you can also just fork the repo! Not any possible scenario does need to be fix inside the library. I also used https://npmjs.com/package/patch-package a few times with great success myself, to make a minor amendment to a library, which I just needed myself. It can be even simpler than forking.

On Thu, Jul 4, 2019 at 9:59 PM Vildan Softic notifications@github.com wrote:

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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/immerjs/immer/issues/392?email_source=notifications&email_token=AAN4NBFHYMUXXGEJPSMU4RDP5ZJBZA5CNFSM4H3PDLNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZIBZIA#issuecomment-508566688, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBEDNGH3HJQOMHZSPITP5ZJBZANCNFSM4H3PDLNA .

[zewa666]

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]

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.

[Venryx]

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.

That solution sounds good to me. (either that or the arguably more flexible alternative in the subsequent comment)

I can understand wanting to keep the library light, but based on the partial implementation of the "copy-value" option here in this PR, it seems that the added complexity is quite small.

Correct me if I'm wrong, but isn't it only about 7 lines to add the "copy-value" option?

• The 3 lines in the PR, to add the functionality.
• 3 lines in immer.js for a setGetterHandling function.
• 1 line in proxy.js, to import that option from immer.js.

As someone who would make use of this feature, the 7 lines seem worth it to me of course. ^_^

[jackhe16]

When combined with vue2, I want to skip the key __ob__ when shallowCopy, otherwise the produced value can't re reactive normally reference here, or consider 'copy-value-enumerable'

import Vue from 'vue';
import VueCompositionAPI, { watch, reactive } from '@vue/composition-api';
import produce, { setAutoFreeze, setUseProxies, enableES5 } from 'immer';

Vue.use(VueCompositionAPI);

setAutoFreeze(false);
setUseProxies(false);
enableES5();

function test_immer_vue2() {
  const state = reactive({
    a: {
      b: 1,
    },
  });

  const nextStateA = produce(state.a, (draft) => {
    draft.b = 2;
  });

  console.log('propertyNames: ', Object.getOwnPropertyNames(nextStateA));

  const prevA = state.a;
  state.a = nextStateA;

  console.log('__ob__ equal: ', prevA['__ob__'] === nextStateA['__ob__']);

  // need getter/setter + __ob__ to work
  watch(
    () => state.a.b,
    (cur, old) => {
      console.log('watch: ', cur, old); // nothing
    }
  );

  state.a.b = 3;
}

// test_immer_vue2();
// Output
// propertyNames: ['b', '__ob__']
// __ob__ equal: true

// function shallowCopy(base) {
//   ...
//   delete descriptors[DRAFT_STATE];
//   delete descriptors['__ob__'];
//   ...
// }
// test_immer_vue2();
// Output
// propertyNames: ['b']
// __ob__ equal: false
// watch: 3 2