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

getState within @ImmutableContext() returns a draft #483

Open
DympyDev opened this issue Jun 8, 2020 · 4 comments
Open

getState within @ImmutableContext() returns a draft #483

DympyDev opened this issue Jun 8, 2020 · 4 comments

Comments

@DympyDev
Copy link

DympyDev commented Jun 8, 2020

So in our application we use this immer-adapter to help make our state immutable.
However, we recently ran into an issue where our production build breaks our apps.
It seems that everything is a lot stricter in production builds than in development build, which pointed us to a bunch of errors when using the getState method.

The getState method returns an immer draft, instead of, what we expected, a frozen instance of our state.
As you can see at this line:
https://github.com/ngxs-labs/immer-adapter/blob/master/src/lib/core/immer-adapter/common/immutable-state-context.ts#L23
It just creates a draft, casts it to the provided interface, calls it frozen and then returns it.

We tried to recreate this issue in an isolated project, and the results were the same.

So given an action like this, that finds an object (Dossier) in an array (dossiers) from the state, based on the dossierNumber:

@Action(DossiersActions.DeleteDossierByNumber)
@ImmutableContext()
public deleteDossierByNummer({ getState, setState }: StateContext<DossiersStateModel>, { dossierNumber }: DossiersActions.DeleteDossierByNumber): void {
  const found = getState().dossiers.find(d => d.dossierNumber === dossierNumber);
  console.log('found', found);
  if (!found) {
    throw new Error(`Could not find existing dossier to delete, for nummer ${dossierNumber}`);
  }
  setState((draft: DossiersStateModel) => {
    console.log('draft', draft)
    delete draft.dossiers[draft.dossiers.indexOf(found)];
    draft.dossiers = draft.dossiers.filter(dossier => !!dossier);
    return draft;
  });
}

We will receive an output like this, and the state would not have been modified (as the found object is nog present in the dossiers list):

found Proxy { ... }
draft Proxy { ... }

However, we would expect something like this (along with the found object to be removed from the dossiers list):

found Dossier { ... }
draft Proxy { ... }

If a working example is required, please do tell and I will try to provide a stack blitz as soon as possible.

@splincode
Copy link
Member

You can send pull request for fixed issue

@DympyDev
Copy link
Author

DympyDev commented Jun 8, 2020

I understand, and I would love to do that, but some guidance would be appreciated as for why the getState returns a draft right now.

From what I can see, the getState returns a draft and sets the frozenState variable, which is later on used to check if the changes need to be finalized in the setState implementation.

However, I wonder if this is intended behaviour.

Wouldn't it be better to have a getState and a separate getDraft method, or something like that.
This way, the default StateContext behaviour for the getState method wouldn't change when using immer (it would just return an immutable version of the state using immers castImmutable or something like that). And if you do want to manipulate the state, you can use the getDraft method, mutate away, and provide that to the setState method, where it will continue using the same behaviour as it does now.
In order to be able to use this "new" getDraft method, it might be wise for the apps to use the ImmutableStateContext interface, instead of the current StateContext interface.

I'm really interested in your thoughts about all this.
If you agree with them I'l happily create a PR attempting to do what I just described :)

@splincode
Copy link
Member

To be honest, I don’t remember why I did this)) this project was experimental and I decided that using decorators was bad and unreadable

I recommended use native produce from immer instead ngxs-labs/immer-adapter

 ctx.setState(produce((state) => {
     // your mutation
 }));

@pdemilly
Copy link

I have been using extensively produce(ctx, draft => { ... }) in my code and it works great but have been declared deprecated in recent built. Is it replaced by ctx.setState(produce(state) => { });

Any pitfall I should be aware when changing it?

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

No branches or pull requests

3 participants