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

Fix: arrow function scope strictness (take 2) #52

Merged
merged 3 commits into from
Mar 15, 2019
Merged

Fix: arrow function scope strictness (take 2) #52

merged 3 commits into from
Mar 15, 2019

Conversation

futpib
Copy link
Contributor

@futpib futpib commented Mar 2, 2019 •
edited
Loading

Fixes #49

This has the original (broken) commit from #50 cherry-picked, and a separate a4ee79e commit fixing the .length error from #51.

I found linking (npm link) eslint-scope into eslint and running eslint's unit tests helpful, maybe this could be part of the eslint-scope's CI checks?

mysticatea
mysticatea reviewed Mar 6, 2019
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

Largely LGTM, but I have a small suggestion.
Would you modify it?

lib/scope.js Outdated Show resolved Hide resolved
@futpib @mysticatea
Apply suggestion from review
d801737 
Co-authored-by: Toru Nagashima <star.ctor@gmail.com>
platinumazure
platinumazure approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! (Would like to get feedback from mysticatea and maybe other team members before merging.)

mysticatea
mysticatea approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

I also confirmed that ESLint tests passed.

@ilyavolodin
Copy link
Member

Can this PR be merged at this point? Or do we need some more reviews?

@not-an-aardvark
Copy link
Member

Seems mergeable to me. We should plan to do another eslint-scope release along with the next ESLint release.

@not-an-aardvark not-an-aardvark merged commit 299df64 into eslint:master Mar 15, 2019
@not-an-aardvark
Copy link
Member

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mysticatea mysticatea mysticatea approved these changes

@platinumazure platinumazure platinumazure approved these changes

Assignees
No one assigned
Labels
accepted bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ArrowFunctionExpression's scope is always strict scope
5 participants
@futpib @ilyavolodin @not-an-aardvark @platinumazure @mysticatea