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

Inform users about immer workings for complex objects #809

Closed
thibthib18 opened this issue Jan 12, 2023 · 4 comments · Fixed by #811
Closed

Inform users about immer workings for complex objects #809

thibthib18 opened this issue Jan 12, 2023 · 4 comments · Fixed by #811

Comments

@thibthib18
Copy link

Easy-peasy documentation is implying that mutations can be done on all parts of the state, without specifying that they are some restrictions for Classes, Maps and Sets

I lost a few hours figuring out that my mutations on a class instance weren't triggering a new rendering because it was missing [immerable]=true. The documentation about modifying the state could be updated to warn other users about this.

Something like:
"/!\ While this applies to array and plain JS objects, other objects may not be immutable by default. Refer to immer documentation for more info"

Many thanks for this library and your work.

@no-stack-dub-sack
Copy link
Collaborator

no-stack-dub-sack commented Jan 13, 2023

Thanks for the report @thibthib18, I'll try to create a code-sandbox to reproduce this soon (which I expect to be able to do from the immer docs and your experience) and then we can go from there on determining the best way to inform easy-peasy users about this.

While it doesn't seem like it would be desirable in your case, just also noting that the ability to disable immer per action was added in #781 and is available with the 5.2.0 release, however full documentation of this may not be available at the moment / the TypeScript types may not fully reflect this change. But feel free to check out the PR if this is something you're interested in.

@AnsonH
Copy link

AnsonH commented Jan 13, 2023

I encountered the same issue a few months ago and I posted a self-answering Stack Overflow question:

easy-peasy: useStoreState not working with ES6 class instances

I have included some Code Sandbox examples in the answer. Hope you'll find them useful during your investigation @no-stack-dub-sack :)

@no-stack-dub-sack
Copy link
Collaborator

no-stack-dub-sack commented Jan 13, 2023

Thanks @AnsonH! Slightly modified version of your sandbox here:

Edit Easy Peasy w/ ES6 classes (forked)

This sandbox illustrates the issue, and provides two possible solutions for getting this to work with classes.

The first, as the OP said, and as noted in your SO post, is to use the immerable symbol to mark the class as compatible with Immer:

import { immerable } from "immer";

class Person {
  [immerable] = true;

  constructor(name, age) {
    this.name = name;
    this.age = age;
  }
}

The second, available to users of easy-peasy 5.2.0 and greater, is to disable Immer and manage immutability for the action yourself:

  updatePersonName: action(
    (state, payload) => {
      return {
        ...state,
        person: new Person(payload, state.person.age),
      };
    },
    { immer: false }
  )

Thanks again for the report @thibthib18, I'll spin up a PR to call out this issue in the docs.

@jmyrland
Copy link
Collaborator

Easy-peasy documentation is implying that mutations can be done on all parts of the state, without specifying that they are some restrictions for Classes, Maps and Sets

As for Maps and Sets, you'd need import easy-peasy/map-set-support as stated here: https://easy-peasy.dev/docs/introduction/browser-support.html#supporting-map-or-set-within-your-state

That being said, those docs are quite hidden - and we should probably add a reference to it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants