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

Freeze context object #4495

Closed
mbernardy opened this Issue Nov 20, 2015 · 10 comments

Comments

Projects
None yet
6 participants
@mbernardy
Copy link

mbernardy commented Nov 20, 2015

I don't know if this is an issue with eslint or the react plugin but since my setup broke between using eslint@1.9.0 and 1.10.0 I figured here is probably the right place.

With the relevant sections of my package.json

"scripts" : {
    ...
    "lint" : "eslint src"
    ...
},
...
"devDependencies" : {
    ...
    "eslint": "1.9.0",
    "eslint-config-airbnb": "^1.0.0",
    "eslint-plugin-react": "^3.9.0",
    ...

eslint installs as version 1.9.0 (obviously) and npm run lint runs fine.

If I change the dependencies (don't touch the lint command) to

    "eslint": "^1.9.0",
    "eslint-config-airbnb": "^1.0.0",
    "eslint-plugin-react": "^3.9.0",

npm installs eslint 1.10.0. Then, running npm run lint gives the following error

[max] $ npm run lint

> eslint src

/home/max/relay/god-site/node_modules/eslint/lib/eslint.js:706
                    throw ex;
                    ^

TypeError: Error while loading rule 'react/no-multi-comp': Can't add property react, object is not extensible
    at Function.componentRule (/home/max/relay/god-site/node_modules/eslint-plugin-react/lib/util/Components.js:111:17)
    at /home/max/relay/god-site/node_modules/eslint/lib/eslint.js:692:28
    at Array.forEach (native)
    at EventEmitter.module.exports.api.verify (/home/max/relay/god-site/node_modules/eslint/lib/eslint.js:671:16)
    at processText (/home/max/relay/god-site/node_modules/eslint/lib/cli-engine.js:230:27)
    at processFile (/home/max/relay/god-site/node_modules/eslint/lib/cli-engine.js:270:18)
    at executeOnFile (/home/max/relay/god-site/node_modules/eslint/lib/cli-engine.js:617:23)
    at Array.forEach (native)
    at CLIEngine.executeOnFiles (/home/max/relay/god-site/node_modules/eslint/lib/cli-engine.js:640:56)
    at Object.cli.execute (/home/max/relay/god-site/node_modules/eslint/lib/cli.js:162:95)

I fixed this by just locking in 1.9.0 but I wanted to make sure y'all were aware of it. Please let me know if there's anything else you need from me. Thanks!

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 20, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 20, 2015

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Nov 20, 2015

With eslint 1.10.0 we freeze the context parameter that gets passed to each rule. With that being said the react plugin is trying to add more properties to the context object for each rule.

I think thats what is happening.

Eslint Reference: https://github.com/eslint/eslint/blob/master/lib/rule-context.js#L85

React Reference: https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/util/Components.js#L105-L111

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 20, 2015

Eesh, didn't realize the react plugin was extending context. It was really not our intention to allow plugins to add arbitrary properties to context, as they could collide with existing or future properties.

@yannickcr Can you give us some insights into what you were trying to accomplish with adding this property to context?

@nzakas nzakas added core question and removed triage labels Nov 20, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 20, 2015

In the meantime, I'm going to revert the change that introduced this issue as it was done primarily for performance reasons. We can reintroduce it for v2.0.0 as a breaking change, and that will give us time to figure out a better way for eslint-plugin-react to do what it wants to do.

@nzakas nzakas added the breaking label Nov 20, 2015

nzakas added a commit that referenced this issue Nov 20, 2015

@nzakas nzakas added this to the v2.0.0 milestone Nov 20, 2015

nzakas added a commit that referenced this issue Nov 20, 2015

Merge pull request #4498 from eslint/issue4495
Fix: Revert freezing context object (refs #4495)
@yannickcr

This comment has been minimized.

Copy link
Contributor

yannickcr commented Nov 20, 2015

Can you give us some insights into what you were trying to accomplish with adding this property to context?

The idea was to expose some react-specific utility functions for the rules, but yes, it was not a great idea to put them in the context. yannickcr/eslint-plugin-react#324 seems to correctly fix the problem, I will review this and certainly publish a patch release in the following hour.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 20, 2015

We've also pushed out a fix - we didn't realize this would be a breaking change, so we reverted it. We'll reintroduce in v2.0.0. Since it looks like you have a better solution already, we're all looking good now. :)

@yannickcr

This comment has been minimized.

Copy link
Contributor

yannickcr commented Nov 20, 2015

I just published eslint-plugin-react 3.10.0 that should fixes the problem.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 21, 2015

@nzakas I think we should close this issue and create another one to add performance optimization back for 2.0, since this issue has been already addressed.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 21, 2015

I'd like to keep this open as the tracking issue for adding it back in and documenting it as a breaking change. This way, we can keep all related commits together through one issue in case we need to track this down again. Maybe we can just change the title of this issue?

@nzakas nzakas changed the title eslint-plugin-react@3.9.0 no longer works with eslint@1.10.0 Freeze context object Nov 23, 2015

benmosher added a commit to benmosher/eslint-plugin-import that referenced this issue Nov 25, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 1, 2015

Working on this.

@nzakas nzakas closed this in 154c203 Dec 1, 2015

nzakas added a commit that referenced this issue Dec 1, 2015

Merge pull request #4588 from eslint/issue4495
Breaking: Freeze context object (fixes #4495)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.