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

Allow regular expressions in no-param-reassign ignorePropertyModificationsFor option #11621

Closed
smably opened this issue Apr 16, 2019 · 1 comment
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon

Comments

@smably
Copy link
Contributor

smably commented Apr 16, 2019

Background

immer is a widely used library (10k GitHub stars, 1.5m weekly npm downloads) for working with immutable state. immer works by taking in an object, creating a temporary version of the object wrapped in a proxy, recording mutations to the temporary object, and returning a new object with the mutations applied (leaving the original unmodified). For example:

import "produce" from immer;

const foo = { bar: false };

const newFoo = produce(foo, (draftFoo) => {
  draftFoo.bar = true;
})

// foo.bar === false
// newFoo.bar === true

Since immer operates by mutating a "draft" version of the object passed in as a parameter, it falls afoul of no-param-reassign with props: true. The author of the library recommends setting ignorePropertyModificationsFor: "draft" (and always using draft as the parameter name) or disabling the props option altogether.

Proposal

In the codebase I work on, I'd like the option to use a more descriptive variable name than draft. The convention I've used is to always prefix the name of the parameter with draft. For example, produce(x, (draftX) => { draftX.y = 'z'; }). In general, I'd like to forbid assignment to properties of parameters, except when the parameter name matches /^draft/. However, ignorePropertyModificationsFor only accepts an array of string literals.

Many other rules accept arrays of regex patterns, and it seems that regular expressions were considered in the proposal for ignorePropertyModificationsFor but were not implemented because the originally proposed options schema was deemed to be too complex.

I would like to propose one of two changes:

Option 1: Change ignorePropertyModificationsFor to take an array of regular expressions instead of an array of strings. This would be a breaking change.

Option 2: Add a new option (ignorePropertyModificationsForPatterns? or something less unwieldy if anyone has a better idea) that takes an array of regular expressions and optionally deprecate ignorePropertyModificationsFor in the next major release.


What rule do you want to change?
no-param-reassign

Does this change cause the rule to produce more or fewer warnings?
Fewer, assuming the same option name is reused, since a literal string would be interpreted as a regex and thus matched against substrings of parameter names, so potentially more parameters would have their property modifications ignored.

How will the change be implemented? (New option, new default behavior, etc.)?
Either a change to the behaviour of ignorePropertyModificationsFor (breaking) or a new option (non-breaking) and potential deprecation of ignorePropertyModificationsFor.

Please provide some example code that this change will affect:

/* eslint no-param-reassign: ["error", { "props": true, "ignorePropertyModificationsFor": ["^draft"] }] */
const newFoo = produce(foo, (draftFoo) => {
  draftFoo.bar = true;
})

What does the rule currently do for this code?
Error: Assignment to property of function parameter 'draftFoo'

What will the rule do after it's changed?
No error because draftFoo matches regex /^draft/.

Are you willing to submit a pull request to implement this change?
Yes.

@smably smably added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Apr 16, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label May 17, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 14, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon
Projects
None yet
Development

No branches or pull requests

1 participant