Skip to content

Do we want to recommend the no-param-reassign eslint rule in the docs? #521

Closed

Description

@phryneas

I just had this idea when another user tried to assign to the state function argument.
There is actually an eslint rule for that:
no-param-reassign

I'd suggest we either add that rule as a recommendation somehwere in the docs or go even one step farther and create a shareable config package .

That way we could add some more recommended rules in the future.

I'm thinking of later-on adding a self-written eslint-typescript rule that disallows assigning to anything of the type Draft or something similar.

Activity

  1. changed the title [-]Do we want to recommend the `no-param-reassign` rule in the docs?[/-] [+]Do we want to recommend the `no-param-reassign` eslint rule in the docs?[/+] on Apr 24, 2020
  2. markerikson commented on Apr 24, 2020

    @markerikson
    Collaborator

    Pretty sure that rule also forbids mutating someParam.field = 123, which doesn't work well with Immer.

  3. phryneas commented on Apr 24, 2020

    @phryneas
    MemberAuthor

    Only if you override the default configuration of { "props": false } to { "props": true }.
    Over in immerjs/immer#189, mwestrate essentially recommends the rule in its default config.

  4. elramus commented on May 6, 2020

    @elramus

    Long time user of redux here, new to RTK / Immer. Running into ESLint complaining that I'm reassigning state in createSlice. But this is fine though, right, because RTK is using produce under the hood? If so, how do you recommend using the no-param-reassign rule? I know you can do ignorePropertyModificationsFor, but in some cases I'd like to just completely rewrite state in the reducer, not just a property, for example if we're holding a loading on/off boolean.

    const searchingSlice = createSlice({
      name: 'searching',
      initialState: false,
      reducers: {
        setStatus: (state, action: PayloadAction<boolean>) => {
          state = action.payload  // Error
        },
      },
    })
    
  5. markerikson commented on May 6, 2020

    @markerikson
    Collaborator

    Assigning state = never does anything useful. All that does is change what the local variable state is pointing to, which is always a no-op.

    If you want to return a completely new state value / replace the existing state, you need to actually return it: return action.payload.

  6. elramus commented on May 6, 2020

    @elramus

    Ohhh okay, thanks much.

  7. phryneas commented on May 6, 2020

    @phryneas
    MemberAuthor

    And that's exactly why I suggest recommending this eslint rule xD

  8. markerikson commented on May 6, 2020

    @markerikson
    Collaborator

    So what does it mean to "suggest" it in this case, exactly? Where would we document that info? How would we encourage people to do so?

  9. phryneas commented on May 6, 2020

    @phryneas
    MemberAuthor

    Hmm, that's the question.

    Maybe in the style guide, hand-in-hand with the immer and RTK recommendations, and/or in the documentation for createReducer and createSlice?

  10. markerikson commented on May 6, 2020

    @markerikson
    Collaborator

    That could work, yeah.

  11. StuartMorris0 commented on Jun 9, 2020

    @StuartMorris0

    I'm interested to know what the proposed solution/information to be in the style guide would be here.

    If you use the standard eslint setup and use an example from RTK such as:

    const issuesDisplaySlice = createSlice({
      name: 'issuesDisplay',
      initialState,
      reducers: {
        displayRepo(state, action: PayloadAction<CurrentRepo>) {
          const { org, repo } = action.payload
          state.org = org                                     // <-- Error
          state.repo = repo                                 // <-- Error
        },
    

    You would get the reassignment errors on the state. lines. What is the proposed solution here? Thanks

  12. phryneas commented on Jun 9, 2020

    @phryneas
    MemberAuthor

    @StuartMorris0 that is most definitely not the standard eslint setup, as the props option to that rule defaults to false.
    Are you extending some overly restrictive eslint config like the airbnb config? (Which in my personal opinion has outlived it's usefulness and is not a good default).

    A correct configuration would be just no-param-reassign: "error" or no-param-reassign: ["error", { "props": false }] (which is equalivalent)

  13. StuartMorris0 commented on Jun 9, 2020

    @StuartMorris0

    You're right it is the airbnb extends causing the issue, I have reverted the rule to its default.
    "no-param-reassign": ["error", { "props": false }]

    Thanks

  14. markerikson commented on Jun 9, 2020

    @markerikson
    Collaborator

    Airbnb has outlived its usefulness and is not a good default

    Yes, this, 💯 :)

  15. StuartMorris0 commented on Jun 10, 2020

    @StuartMorris0

    Does anyone know of any other good defaults I could review please?

  16. markerikson commented on Jun 10, 2020

    @markerikson
    Collaborator
  17. BenjiTheC commented on Nov 15, 2020

    @BenjiTheC

    Airbnb has outlived its usefulness and is not a good default

    Yes, this, 💯 :)

    If Airbnb rule set has outlived its usefulness, is there a newly recommended one? Thx!

  18. phryneas commented on Nov 15, 2020

    @phryneas
    MemberAuthor

    @BenjiTheC what about the defaults that ship with eslint?
    Or the one linked directly above your question.

  19. ackalhan commented on Dec 30, 2020

    @ackalhan

    What I normally do is

    • All my reducers are in a single directory called reducers.
    • Then I want to force the eslint to disable no-param-reassign for the reducers directory only.
    • Then I create a new eslint config file(.eslintrc.json) inside the reducers directory, and add "no-param-reassign": ["error", { "props": false }] to it.

    so the .eslintrc.json inside reducers directory is like below

    {
      "rules": {
        "no-param-reassign": ["error", { "props": false }]
      }
    }
    
  20. markerikson commented on Mar 23, 2021

    @markerikson
    Collaborator
  21. stevedeighton commented on Aug 19, 2022

    @stevedeighton

    l'm a bit late to the party but I create my slices with naming convention of x.slice.ts, then use an override to only apply this rule to slices:

    { "files": ["src/**/*.slice.ts"], "rules": { "no-param-reassign": "off" } }

  22. Luk-z commented on Oct 13, 2022

    @Luk-z
    Contributor

    l'm a bit late to the party but I create my slices with naming convention of x.slice.ts, then use an override to only apply this rule to slices:

    { "files": ["src/**/*.slice.ts"], "rules": { "no-param-reassign": "off" } }

    better solution, update the docs with this logic ?

  23. MikelArnaiz commented on Dec 22, 2022

    @MikelArnaiz

    l'm a bit late to the party but I create my slices with naming convention of x.slice.ts, then use an override to only apply this rule to slices:

    { "files": ["src/**/*.slice.ts"], "rules": { "no-param-reassign": "off" } }

    You should probably not turn off the rule completely, otherwise it won't warn you when reassigning state. What I have is:

    // @filename .eslintrc.js
    module.exports = {
      ...
      overrides: [
          {
            files: ['scr/**/slice.ts'],
            rules: { 'no-param-reassign': ['error', { props: false }] },
          },
        ],
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions