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

Why assignment to property of function parameter is bad style. #1217

Closed
zhaoxiongfei opened this issue Dec 21, 2016 · 14 comments · Fixed by #1325
Closed

Why assignment to property of function parameter is bad style. #1217

zhaoxiongfei opened this issue Dec 21, 2016 · 14 comments · Fixed by #1325

Comments

@zhaoxiongfei
Copy link

zhaoxiongfei commented Dec 21, 2016

Node.js web service

(req, res, next) => {
  req.user = Guest;
  return next();
};

Often used, can not be avoided.
How should I do?

@ljharb
Copy link
Collaborator

ljharb commented Dec 21, 2016

Since express does require this, you currently can only disable the rule entirely, on a per-file basis, or on a per-line/per-function basis.

eslint has an option proposed to cover this: eslint/eslint#6505

Once that's enabled, we'll likely enable it by default to cover req, request, and $scope.

@zhaoxiongfei
Copy link
Author

Thank you sir.

@ljharb
Copy link
Collaborator

ljharb commented Dec 27, 2016

(the question's answered, but reopening to track the eslint rule change)

@ljharb ljharb reopened this Dec 27, 2016
@felixsanz
Copy link
Contributor

felixsanz commented Dec 30, 2016

@zhangchitc You are not reassigning the parameter, you are just creating a property on the object.

Because of this, you can allow it today with "no-param-reassign": ["error", { "props": false }],.

@ljharb
Copy link
Collaborator

ljharb commented Dec 30, 2016

Yes, but "props": true is something that should be strictly enforced everywhere, except where a framework requires it, like express. The eslint rule change would be for "props": true, but with exceptions for req, etc.

@christianbundy
Copy link
Contributor

Just a quick update, this has been merged and will be released in the next version of ESLint (which I believe is tomorrow): eslint/eslint#6505 (comment)

@shanev
Copy link

shanev commented Mar 7, 2017

Another example is ctx in Koa routing:

router.get('/', (ctx) => {
  ctx.status = 200;
});

@ljharb
Copy link
Collaborator

ljharb commented Mar 16, 2017

The next release of eslint-config-airbnb-base, and likely eslint-config-airbnb, will include this loosening of no-param-reassign.

@jbruni
Copy link

jbruni commented Nov 7, 2017

no-param-reassign as "never reassign parameters" is ok.

no-param-reassign as "never mutate parameters" means eslint-config-airbnb enforces immutability paradigm. It makes a profound design choice for us.

@ljharb
Copy link
Collaborator

ljharb commented Nov 7, 2017

@jbruni Airbnb's config makes many profound design choices for you; that's the point. If you don't agree with them, you can easily override rules and/or fork the guide.

@kholiavko-roman
Copy link

kholiavko-roman commented Feb 14, 2019

Guys does anybody knows how to set up eslit for showing error if I modify value from param array ?
Here is example:

const checkIsValid = (id, array) => {
  for (let i = 0; i < array.length; i++) {
    const item = array[i];

    if (item.param) {
      item.value = null;
      item.payment = null;
    }
  }

  return array;
};

@ljharb
Copy link
Collaborator

ljharb commented Feb 14, 2019

@kholiavko-roman that’s pushing the limits of static analysis; you may want to try eslint-plugin-immutable, but it’s not very reliable due to the way the language works.

@aboyton
Copy link

aboyton commented Feb 15, 2019

@kholiavko-roman Or try TypeScript and something like jonaskello/tslint-immutable.

@kholiavko-roman
Copy link

Thanks for reply. I think I will try eslint-plugin-immutable for the first.

makevoid added a commit to appliedblockchain/eslint-config that referenced this issue Aug 12, 2019
Otherwise with koa context (`ctx`) it's too tedious.

I'm happy to use `ignorePropertyModificationsFor` and whitelist `ctx` and other common names as a strictier alternative

infos:
- https://eslint.org/docs/rules/no-param-reassign
- airbnb/javascript#1217 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants