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

Usage with eslint no-param-reassign #189

Closed
brummelte opened this issue Sep 14, 2018 · 16 comments
Closed

Usage with eslint no-param-reassign #189

brummelte opened this issue Sep 14, 2018 · 16 comments

Comments

@brummelte
Copy link

brummelte commented Sep 14, 2018

I don't want to disable the eslint rule "no-param-reassign" globally, because it prevents errors.

So this seems to be the best way to use immer with that rule enabled:

const nextState = produce(baseState, (draftState) => {
    /* eslint-disable no-param-reassign */
    draftState.test = "New value";
    /* eslint-enable no-param-reassign */
});

There is a way to specify "ignorePropertyModificationsFor" for that eslint rule (https://eslint.org/docs/rules/no-param-reassign). So draftState could be ignored. But that would only exclude that name.

Is there a better way without disabling that rule?

@mweststrate
Copy link
Collaborator

@brummelte I don't think your example is a param reassign and the rule shouldn't trigger on it

@brummelte
Copy link
Author

You are correct, I just copied it from your example. But the question stays the same:

const nextState = produce(baseState, (draftState) => {
    /* eslint-disable no-param-reassign */
    draftState.test = "New value";
    /* eslint-enable no-param-reassign */
});

@mweststrate
Copy link
Collaborator

mweststrate commented Sep 14, 2018 via email

@brummelte
Copy link
Author

brummelte commented Sep 14, 2018

It is with the option { "props": true }.

Which the airbnb preset uses: https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/best-practices.js

image

@mweststrate
Copy link
Collaborator

mweststrate commented Sep 14, 2018 via email

@mweststrate
Copy link
Collaborator

mweststrate commented Sep 14, 2018 via email

@davalapar
Copy link

I also don't like disabling it within its file or function scope.

I think // eslint-disable-line no-param-reassign, although ridiculously repetitive, would do the work.

@brummelte
Copy link
Author

brummelte commented Sep 15, 2018

For future readers: did that option solve the issue?

The issue was, that that is not the perfect solution. But yes, that setting would work.

@mweststrate
Copy link
Collaborator

Ok, to summarize:

  1. no-param-reassign rule makes perfect sense, I actually strongly recommend it, because draft = X assignments are really a no-op
  2. props: true option though kinda defeats the purpose of immer, and should be kept to false (the default), or an exception could be made by setting ignorePropertyModificationsFor: "draft"

@jaimefps
Copy link

Just to make it quicker for ya'll,
if you are extending airbnb,
just do the following in your eslint.rc file:

module.exports = {
  extends: [
    'airbnb', // this means you have "props" = "true" by default
  ],
  rules: {
    'no-param-reassign': ['error', { props: true, ignorePropertyModificationsFor: ['draft'] }],
  },
};

@nurbek-ab
Copy link

Also in case you have multiple producers in the same scope, like this:

  markAsSubmitted = isLoading => {
    this.setState(produce(draft => {
      draft.loading = isLoading;
      draft.secondLevel = draft.secondLevel.map(produce(sketch => {
        sketch.submitted = true;
        sketch.thirdLevel = sketch.thirdLevel.map(produce(outline => {
          outline.submitted = true;
        }));
      }));
    }));
  };

This rule would work:

    "no-param-reassign": ["error", {
      "props": true,
      "ignorePropertyModificationsFor": ["draft", "sketch", "outline"]
    }],

@SpotHimesh
Copy link

For people coming from future google searches, extending on @jaimefps comment above

To enable more semantic variable naming as well as avoiding problem in case of multiple producers in the same scope, we can do

module.exports = {
  extends: [
    'airbnb', // this means you have "props" = "true" by default
  ],
  rules: {
    'no-param-reassign': ['error', { props: true, ignorePropertyModificationsForRegex: ["^draft"] }],
  },
};

This should make it work for draft, draftPost, draftState

@zedelashvililevani
Copy link

Another trick that works:

const nextState = produce(baseState, (draft) => {
    const tempDraft = draft;
    tempDraft .test = "New value";
});

@Crismon96
Copy link

Crismon96 commented May 20, 2021

So the rule in .eslintrc from you guys helped me to fix this problem in my immer code:

rules: {
     "no-param-reassign": [1, { "props": true, "ignorePropertyModificationsFor": ["state"] }]
  },

But if I want to reassign the state completely, like in this reducer (working with redux toolkit in this project):

        incrementJoint: (state: RobotModel, action: PayloadAction<RobotModelJoint>) => {
            state = someNewStateValue
        },

Giving me the infamous linter warning
Assignment to function parameter 'state'.eslintno-param-reassign

Do you guys know why it works with reassigning nested state but not the complete state or what I can do different?
I fallback to disabling the eslint rule for this line for now but I would rather not do that in this case.

@ljharb
Copy link

ljharb commented May 20, 2021

@Crismon96 in some JS engines, reassigning to a named argument heavily deoptimizes the function. Always make a new variable name instead.

@daliborfilus
Copy link

That rule triggered for me in immer (that's how I landed on this issue), but also on code like this:

export function groupBy(data, fnc) {
  return data.reduce((acc, curr) => {
    const res = fnc(curr);
    if (!acc[res]) acc[res] = [curr];
    else acc[res].push(curr);
    return acc;
  }, {});
}

75:20 error Assignment to property of function parameter 'acc' no-param-reassign

So now I can't even use reduce anymore? This rule (with props: true) is just stupid.
I think I'll just set props to false.

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

10 participants