-
Notifications
You must be signed in to change notification settings - Fork 2k
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
test circleci transform-class #24308
Conversation
@nosolosw: do you know if I will accidentally introduce any errors to |
I'm not sure why, but it seems like the version of I'll do some investigating to try and figure out why these were getting past eslint |
I figured out the explanation for why this upgrade creates new errors: It turns out that function foo( x, y ) {
return x; // error: variable y is unused
}
function foo( x, y ) {
return y; // no errors here
} They called this property Up until recently, eslint had a bug where they weren't treating unused destructured args as errors if there were args that occur after it, which doesn't make much sense: function foo( { x, eatY }, y ) {
return eatY( y ); // would not error but should report x as unused!!
} This was fixed recently: eslint/eslint#8442 |
This reverts commit 5abf6fc.
Confirmed that eslines is no longer yelling and the fixes seem to work. The build backs that up. I think this is ok to go? Unsure what else to test. There's so much noise in a plain eslint run that it's hard to know if there's a diff. |
a plain on this branch:
b3c182b, the commit this branch is forked from:
c24d721, the commit before babel-7:
so looks like we're back to the old ballpark... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a local change with this branch by adding a class property to a component.
Before npm install
:
[~/checkouts/wp-calypso] (try/transform-class-props)$ git commit -am 'wip'
Found '/Users/kerryliu/checkouts/wp-calypso/.nvmrc' with version <v8.11.0>
Now using node v8.11.0 (npm v5.6.0)
> husky - npm run -s precommit
By contributing to this project, you license the materials you contribute under the GNU General Public License v2 (or later). All materials must have GPLv2 compatible licenses — see .github/CONTRIBUTING.md for details.
Prettier formatting file: client/me/help/help-contact/index.jsx because it contains the @format flag
/Users/kerryliu/checkouts/wp-calypso/client/me/help/help-contact/index.jsx
91:2 error 'foo' is not defined no-undef
after npm install
:
[~/checkouts/wp-calypso] (try/transform-class-props)$ git commit -am 'wip'
Found '/Users/kerryliu/checkouts/wp-calypso/.nvmrc' with version <v8.11.0>
Now using node v8.11.0 (npm v5.6.0)
> husky - npm run -s precommit
By contributing to this project, you license the materials you contribute under the GNU General Public License v2 (or later). All materials must have GPLv2 compatible licenses — see .github/CONTRIBUTING.md for details.
[try/transform-class-props 262fa36828] wip
1 file changed, 2 insertions(+)
Note that I also see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Errors still work as expected.
I tested this PR on a patch where I'm adding a new React component with class properties and it works. I was getting linting errors with old ESLint, but these disappeared after the upgrade. There is one warning during
Can we bump the peer dependency in |
@jsnajdr looks like a Trying that out. |
moved to |
Hey, sorry for not being able to help here - I was AFK until today. As @jsnajdr mentioned: I had to revert the ESLint v4 upgrade due to errors with class properties. Problem was that babel-eslint didn't support ESLint v4 at the time. I just checked and they changed that a few days ago. So the only thing needed for this PR to work was to update the Closing #18106 as this PR has addressed it. |
seeing if class properties break circleLooks like class properties were breaking.
summary
While working on a calypso pull request, @Copons noticed that newly introduced class properties were failing eslint. Linting is part of our standard automated testing, and so any branches that add in new (or modify old) class properties were failing on those lines, and also skipping unit tests.
The introduction of this error is probably related to the babel 7 upgrade: #23424
to repro locally
node bin/eslint-branch.js
.how this pr fixes it
upgrades eslint, eslines, and babel-plugin-react.
We came to that conclusion after reading a couple related issues:
Note: I thought we had fixed a similar issue awhile back (I did). I believe it came back with babel 7.
cc @Copons @blowery @gwwar