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

Why can't drafts have computed properties? #317

Closed
Krantisinh opened this issue Feb 15, 2019 · 12 comments

Comments

Projects
None yet
7 participants
@Krantisinh
Copy link

commented Feb 15, 2019

Using immer v1.9.0+ with VueJS gives error 'Immer drafts cannot have computed properties'

  • Issue: _______
    • Version: 1.9.0 onwards
    • Expected behavior: Updating state using immer produce function runs without errors.
    • Observed behavior: Immer (1.9.0+), when used with VueJS (2.5.0+) throws an error 'Immer drafts cannot have computed properties' while updating the state.

On further analysis, it looks like -
Vue JS sets getters in all properties used in its components by default.
And when we looked at the source of immer, we found that, below code throws error once it encounters a getter on any prop while making a shallow copy.

Immer is an awesome library and we want to continue using the same, but this issue is blocking us. Could you please explain the rationale behind below change and let us know how we can help to fix this issue ?

export function shallowCopy(base, invokeGetters = false) {
    if (Array.isArray(base)) return base.slice()
    const clone = Object.create(Object.getPrototypeOf(base))
    ownKeys(base).forEach(key => {
        if (key === DRAFT_STATE) {
            return // Never copy over draft state.
        }
        const desc = Object.getOwnPropertyDescriptor(base, key)
        if (desc.get) {
            if (!invokeGetters) {
                throw new Error("Immer drafts cannot have computed properties")
            }
            desc.value = desc.get.call(base)
        }
        if (desc.enumerable) {
            clone[key] = desc.value
        } else {
            Object.defineProperty(clone, key, {
                value: desc.value,
                writable: true,
                configurable: true
            })
        }
    })
    return clone
}
@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

@Krantisinh why are you using immer with VueJS objects? VueJS are designed to be mutable so that they can be tracked by Vue, and Immer is trying to achieve to do exactly the opposite, so using immer on these kind of objects doesn't really add any value?

The problem with getters is that they can't be cloned. What should be cloned? The value? But in that case they become 'static'. Or should the descriptor be copied? But in that case the closure of the getter might be wrong, that is, referring to something it was already referring to, there is no way to tell..

@aleclarson aleclarson added the question label Feb 15, 2019

@aleclarson aleclarson changed the title Using immer v1.9.0+ with VueJS throws error 'Immer drafts cannot have computed properties' Why can't drafts have computed properties? Feb 15, 2019

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

Note: You can still have computed properties on the prototype of your objects. But you also need to do prototype[immerable] = true to ensure the prototype is preserved across copies.

As @mweststrate says, there's no point in using Immer on Vue objects directly. Perhaps you can provide an example of what you're trying to accomplish, @Krantisinh?

@aleclarson aleclarson closed this Feb 15, 2019

@oriSomething

This comment has been minimized.

Copy link

commented Feb 15, 2019

I think Immer is a valid choice with Vue in some cases. But, I think the issue is with Vue and not Immer, since using Vue is a package deal of Vue modify your objects.

@Krantisinh

This comment has been minimized.

Copy link
Author

commented Feb 19, 2019

@mweststrate @aleclarson We have redux to manage state in VueJS app. And We're using immer to do state changes in the reducers.

Since the traditional methods of changing values in the state end up mutating the state, we felt immer's produce function is our safe bet to do the state changes inside the reducers.

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2019

If you provide a simplified example via CodeSandbox or similar, we can help you out.

@mistyharsh

This comment has been minimized.

Copy link

commented Feb 20, 2019

Thank you all for bringing this up. We are also facing this issue. Here is the sample CodePen to illustrate the problem. There are two stores. The store store1 doesn't user immer for reducer whereas store2 uses produce function.

We never use immer.produce inside Vue.js components. But at some point, Vue.js adds getters to the data coming out of Redux store and the problem happens.

Using Redux and Vue together is a bit odd combination but data flows in Redux are more explict than that of Vuex and thus we use Redux. Earlier we used Ramda and spread operator to manage our reducers but Immer really helped us write clean reducers especially when TypeScript comes into the picture. (It is also one of the reason that we cannot use redux-freeze in dev mode.)

Vue.js is doing its job right and same goes for Immer. It is just that when they come together, they don't play well. I wonder if we can have any escape hatch for this.

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2019

@jasonmit

This comment has been minimized.

Copy link

commented Apr 3, 2019

This change also makes it very difficult to integrate with EmberJS because of how Ember sets up setters and getters within its KVO to observe state changes on objects. For example, if I have an immer object and create a computed property (not directly on the immer object) that references a dependent key on the immer object then Ember sets up a getter/setter so that it knows when to recompute that computed property. It's merely shadowing to observe changes, do not do anything to the underlying immer objects value.

The ability to opt out the of assertion and to take the value of the getter would resolve this.

If I find an alternative, I'll circle back and update but for now I don't see a way out of this other than pinning myself to an older version of immer.

@cainlevy

This comment has been minimized.

Copy link

commented May 20, 2019

Pardon my naivety: could getters be skipped rather than cloned?

I'm wondering if this is a necessary design choice, or one built for maximum defensiveness that also happens to preclude otherwise workable systems.

@aleclarson

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

@cainlevy Hmm... Should we add a "strict" mode to Immer (which defaults to false) that throws on getters (and possibly other invariants) when true? Maybe not worth the extra kB? We'll need to figure what a strict mode would protect against specifically so we can gauge the worth properly.

@cainlevy

This comment has been minimized.

Copy link

commented May 20, 2019

My impression was that Immer defaults to strict mode and [immerable] = true was how I disabled that strictness.

@mweststrate

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.