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

globalReturn is not reset when node environment is set to false #3915

Closed
ilyavolodin opened this Issue Sep 23, 2015 · 15 comments

Comments

Projects
None yet
5 participants
@ilyavolodin
Copy link
Member

commented Sep 23, 2015

Using ESLint v1.5.0 and the following directory structure:

..
.eslintrc
src
 - .eslintrc
 - test.js

If the top level .eslintrc file has

{
  "env": { "node": true }
}

and the one inside src directory has

{
  "env": { "node": false, "browser": true }
}

For test.js file environments correctly cascade to node: false and browser: true, however, globalReturn ecmaFeature is still set to true. I don't think that's correct behavior. This can be reproduced by using no-unused-vars, declaring unused variable inside test.js and then adding exported comment with variable name. ESLint will report variable still being unused (will file another issue for this #3916). @mysticatea could you take a look? I think this is related to the fix for #3735 since this is behaving correctly in ESLint v1.4.0

@mysticatea

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

Okay, I will do.

@mysticatea

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

The cause is that createEnvironmentConfig checks only environments which are set true. I had mistaken checking inside of this function.

Now, envs have globals but we cannot turn globals off, so it cannot overwrite globals with env: false.
So I need to rewrite the overwriting-logic.

I will do it this evening (JST)

@ilyavolodin

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2015

Sounds good. Thanks for looking into it.

@mysticatea

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

It's hard.
I will try in a few days.

@nzakas nzakas added the accepted label Sep 24, 2015

@mysticatea mysticatea self-assigned this Sep 25, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Sep 27, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Sep 28, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Sep 28, 2015

@nzakas nzakas assigned nzakas and unassigned mysticatea Dec 11, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 11, 2015

I'll see if I can fix this

@rocketmonkeys

This comment has been minimized.

Copy link

commented Dec 14, 2015

@mysticatea @nzakas - I just ran into this bug and I can see where it's happening. Are either of you currently working on a fix? If not, I might try my hand (since I just spent an hour digging through this code).

@ilyavolodin

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2015

@rocketmonkeys I would submit PR anyways (if it's not going to take a long time to do), since even if somebody else will work on it, they can verify their fix against yours. It's also been 4 days since @nzakas posted his message.

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

I was going to look at it today, but feel free to submit a PR

@rocketmonkeys

This comment has been minimized.

Copy link

commented Dec 15, 2015

I have a fix, but not the tests; those are harder since I'm not nearly as familiar with the mocha/testing setup we have. Can I just PR the fix as is, or do I need the test in there before I do anything?

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

You can PR the fix if you'd like to get feedback, we just can't merge until there are tests.

@rocketmonkeys

This comment has been minimized.

Copy link

commented Dec 15, 2015

@nzakas - ok, prelim PR here: #4713

I'm guessing it's not complete, as this is my first dive into the code. It works for my use-case (parent folder enables env, but subfolder disables it). Not sure about others. I'll work on the test, no idea when I'll have that done.

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

I'm also going to dig into this.

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

The root cause is that we are applying environment settings as files are being loaded, instead of waiting until all merging is complete. So basically, just have to wait until after merging to do that.

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

Now that makes it sound simple 😄
Awesome 👍

@mysticatea

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

please remember this issue: #3735 (comment)

@nzakas nzakas closed this in 8410869 Dec 15, 2015

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

Merge pull request #4714 from eslint/issue3915
Fix: Apply environment configs last (fixes #3915)

@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.