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

Using a immerable class with a getter without a setter #558

Closed
wants to merge 2 commits into from
Closed

Using a immerable class with a getter without a setter #558

wants to merge 2 commits into from

Conversation

dalcib
Copy link
Contributor

@dalcib dalcib commented Mar 23, 2020

In an immerable class with a getter without the corresponding setter, assigning a value to any field using the getter value within a class method causes the following error:

#TypeError#
Cannot set property bar of # which has only a getter

It only happens with non-primitive getter values and with setUseProxies(true)
It can be fixed in userland creating an empty setter (set prop(v) {})
This PR makes the markChangedProxy function to skip the assignment copy[key]=value when there is a getter without a setter.

It fixes #439 and replaces #438

Links to Repro (updated to immer 6.0.2)

https://codesandbox.io/s/small-rain-inotu
https://codesandbox.io/s/useimmer-tic-tac-ci6qq

To Reproduce

import { produce, immerable } from "immer";
class Obj {
  [immerable] = true;
  foo = 0;
  _bar = { baz: 1 };
  get bar() {return this._bar}
  syncFoo() {
    return produce(this, state => {
      state.foo = state.bar.baz;
    });
  }
}
const obj = new Obj();
const obj2 = obj.syncFoo();

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 23, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 081fff1:

Sandbox Source
Immer sandbox Configuration
staging-rain-vybit PR
useImmer - Tic Tac PR

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.

Direction feels solid to me :) left a question and two ideas for better test coverage. Thanks for diving into this!

__tests__/base.js Show resolved Hide resolved
__tests__/base.js Show resolved Hide resolved
src/core/proxy.ts Show resolved Hide resolved
@mweststrate
Copy link
Collaborator

Merged into #593, changed the implementation there to be a bit more efficient, and fix the problem and trap rather than at finalize time, still wip, but seems we can solve some other issues with it as well

@mweststrate mweststrate closed this May 5, 2020
mweststrate added a commit that referenced this pull request Jun 10, 2020
* Introduced `current`, which takes a snapshot of the current state of a draft and finalizes it (but without freezing). Current is a great utility to print the current state during debugging (no Proxies in the way), and the output of current can also be safely leaked outside the producer. Implements #441, #591
* [BREAKING CHANGE] getters and setters are now handled consistently: own getters and setters will always by copied into fields (like Object.assign does), inherited getters and setters will be left as-is. This should allow using Immer directly on objects that trap their fields, like down in Vue or MobX. Fixes #584, #439, #593, #558
* [BREAKING CHANGE] produce no longer accepts  non-draftable objects as first argument
* [BREAKING CHANGE] original can only be called on drafts and will throw otherwise (fixes #605)
* [BREAKING CHANGE] non-enumerable and symbolic fields will never be frozen
* [BREAKING CHANGE] the patches for arrays are now computed differently to fix some scenarios in which they were incorrect. In some cases they will be more optimal now, in other cases less. Especially splicing / unshifting items into an existing array might result in a lot of patches. Fixes #468
* Improved documentation in several areas, there is now a page for typical update patterns and a separate page on how to work with classes. And additional performance tips have been included. Fixes #457, #115, #462
* Fixed #462: All branches of the produced state should be frozen
* Fixed #588: Inconsistent behavior with nested produce
* Fixed #577: Immer might not work with polyfilled symbols
* Fixed #514, #609: Explicitly calling `useProxies(false)` shouldn’t check for the presence of Proxy.
@aleclarson
Copy link
Member

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mweststrate
Copy link
Collaborator

Sorry it took so long @dalcib! This rabbit hole went way deeper to do it entirely correct than I expected, but I think the current solution in Immer 7 should work in general!

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.

Calling an object getter on an immerable draft occurs an Error.
3 participants