Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

Fix escope to take sourceType and ecmaVersion from options #288

Merged
merged 1 commit into from Apr 20, 2016
Merged

Fix escope to take sourceType and ecmaVersion from options #288

merged 1 commit into from Apr 20, 2016

Conversation

danez
Copy link
Member

@danez danez commented Apr 1, 2016

escope was hardcoded to sourcetype: "module" and ecmaVersion: "6"
This changes it to take the configuration from the eslint options and still
defaulting to "module" and "6".
This is done by having the variables in the upper scope, as monkeypatch is only triggered once and would therefore stick to the options that are supplied the first time. But the options can change as in our tests or different eslintrc files in different directories.

Fixes #287 and #217 (217 was failing without the fix in script mode)

@josh
Copy link
Contributor

josh commented Apr 1, 2016

Nice! Thanks @danez

);
});

it("no-implicit-globals in module", function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should add a test for when the parserOptions aren't specified?

@danez
Copy link
Member Author

danez commented Apr 1, 2016

I removed the defaulting to "module" in the test as @hzoo suggested. Then some tests were failing as the default value for sourceType and ecmaVersion was not working correctly.

After fixing that now only one test fails, but I don't have time right now to investigate. Seems there is still a difference between setting sourceType='module' or not setting it.

@josh
Copy link
Contributor

josh commented Apr 1, 2016

After fixing that now only one test fails, but I don't have time right now to investigate. Seems there is still a difference between setting sourceType='module' or not setting it.

Looks like you're referring to the "no-use-before-define #192" test. It looks like @hzoo suggested modules must be used in this case.

escope was hardcoded to sourcetype: "module" and ecmaVersion: "6"
This changes it to take the configuration from the eslint options and still
defaulting to "module" and "6".
This is done by having to global variables, as monkeypatch is only triggered once.
To fix scoping issues, the same logic as in eslint is applied. It disables the nodejs scope
if the sourceType is module.
@danez
Copy link
Member Author

danez commented Apr 4, 2016

Ok I finally had time to look at this and figured out what the problem was with the "no-use-before-define" rule test.
The rule was checking the global scope and its first childScope for use-before-define breakages. But babel-eslint (at least in the tests) was setting nodejsScope = true (from globalReturn) and sourceType module for escope, which created 2 childscopes, one for the nodjes-env and one for the module. And this somehow confused the rule.

Now the nodejsScope is set to false if sourceType is module. Like eslint is doing that:
https://github.com/eslint/eslint/blob/5685ff58d74cb2b7096bcfa1dfc6cae95c2a9278/lib/eslint.js#L834
https://github.com/eslint/eslint/blob/5685ff58d74cb2b7096bcfa1dfc6cae95c2a9278/lib/eslint.js#L449

@josh
Copy link
Contributor

josh commented Apr 7, 2016

This looks good to me!

@danez
Copy link
Member Author

danez commented Apr 12, 2016

@hzoo Do you have time to look at this?

@josh
Copy link
Contributor

josh commented Apr 19, 2016

Friendly bump 😄

@hzoo
Copy link
Member

hzoo commented Apr 20, 2016

Sorry all - busy and maybe didn't want to check all of this 😄 .

@hzoo hzoo merged commit 991ec90 into babel:master Apr 20, 2016
@josh
Copy link
Contributor

josh commented Apr 20, 2016

@hzoo ❤️

@danez danez deleted the fix-escope-monkey branch April 20, 2016 13:51
@hzoo
Copy link
Member

hzoo commented Apr 21, 2016

Also @josh @danez Do yall want to be contributors? (ref to #88)

nicolo-ribaudo pushed a commit to babel/babel that referenced this pull request Nov 14, 2019
…el-eslint#288)

escope was hardcoded to sourcetype: "module" and ecmaVersion: "6"
This changes it to take the configuration from the eslint options and still
defaulting to "module" and "6".
This is done by having to global variables, as monkeypatch is only triggered once.
To fix scoping issues, the same logic as in eslint is applied. It disables the nodejs scope
if the sourceType is module.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants