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

tools: update ESLint to 5.2.0 #21817

Closed
wants to merge 1 commit into from
Closed

tools: update ESLint to 5.2.0 #21817

wants to merge 1 commit into from

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Jul 14, 2018

changelog: https://github.com/eslint/eslint/releases/tag/v5.1.0 https://github.com/eslint/eslint/releases/tag/v5.2.0

CI: https://ci.nodejs.org/job/node-test-pull-request/15882/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@hiroppy hiroppy added the tools Issues and PRs related to the tools directory. label Jul 14, 2018
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 14, 2018
@@ -21,7 +21,7 @@
"_resolved": "https://registry.npmjs.org/acorn-jsx/-/acorn-jsx-4.1.1.tgz",
"_shasum": "e8e41e48ea2fe0c896740610ab6a4ffd8add225e",
"_spec": "acorn-jsx@^4.1.1",
"_where": "/Users/cjihrig/iojs/node/tools/eslint-tmp/node_modules/eslint/node_modules/espree",
"_where": "/Users/about_hiroppy/Programming/nodejs/node/tools/eslint-tmp/node_modules/eslint/node_modules/espree",
Copy link
Member

Choose a reason for hiding this comment

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

It appears that most of the 179 package.json file changes (of the 205 total changed files here) are simply changes like this. This is definitely churn we don't need. Any chance we can omit the package.json files that only change the _where path from the commit? I'll work on a PR to update update-eslint.sh to do this automatically.

Copy link
Member

Choose a reason for hiding this comment

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

(PR: #21819)

Copy link
Member Author

@hiroppy hiroppy Jul 15, 2018

Choose a reason for hiding this comment

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

good catch! thanks @Trott. I will wait for #21819 to be merged.

@Trott Trott mentioned this pull request Jul 15, 2018
2 tasks
@hiroppy
Copy link
Member Author

hiroppy commented Jul 15, 2018

node-test-commit-osx failed but it has no related this PR.

20:24:08 not ok 2340 sequential/test-timers-blocking-callback
20:24:08   ---
20:24:08   duration_ms: 2.301
20:24:08   severity: fail
20:24:08   exitcode: 1
20:24:08   stack: |-
20:24:08     assert.js:80
20:24:08       throw new AssertionError(obj);
20:24:08       ^
20:24:08     
20:24:08     AssertionError [ERR_ASSERTION]: timeout delayed by more than 100% (250ms)
20:24:08         at blockingCallback (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1012/test/sequential/test-timers-blocking-callback.js:62:14)
20:24:08         at ontimeout (timers.js:457:11)
20:24:08         at tryOnTimeout (timers.js:327:5)
20:24:08         at listOnTimeout (timers.js:301:5)
20:24:08         at processTimers (timers.js:258:5)

@Trott
Copy link
Member

Trott commented Jul 15, 2018

@hiroppy It's a known flaky. See #21781. You can use "Resume Build" on the node-test-pull-request page for your CI run to create a new node-test-pull-request that will keep all the green from that run and re-run everything else. (Although I think we'll need a full CI re-run once you remove the package.json files from the commit.)

@Trott Trott added blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. labels Jul 15, 2018
@Trott
Copy link
Member

Trott commented Jul 20, 2018

Change to update-eslint.sh has landed so I've removed the blocked label. This will still have a bigger-than-usual diff because the new update-eslint.sh will remove a bunch of unnecessary stuff from all of the package.json files, but after this lands, that won't happen again. 🎉

@hiroppy hiroppy changed the title tools: update ESLint to 5.1.0 tools: update ESLint to 5.2.0 Jul 23, 2018
@hiroppy
Copy link
Member Author

hiroppy commented Jul 23, 2018

Used the patch created by @Trott and updated ESLint to 5.2.0.

@Trott
Copy link
Member

Trott commented Jul 23, 2018

@Trott
Copy link
Member

Trott commented Jul 26, 2018

@Trott
Copy link
Member

Trott commented Jul 26, 2018

@nodejs/linting This could use another review or so.

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Two points:

  1. This release should bring support for some globals we manually define, so you should be able to remove them.
  2. From the diff, I'm not sure if eslint's dependencies were actually updated because the diff does not contain updated files for globals, for example.

@Trott
Copy link
Member

Trott commented Jul 26, 2018

  1. From the diff, I'm not sure if eslint's dependencies were actually updated because the diff does not contain updated files for globals, for example.

@silverwind Current master branch (with ESLint 5.0.0) has globals 11.7.0, which is the most recent release. So I wouldn't expect an update to ESLint 5.2.0 to change that. It's still going to be globals 11.7.0. (Or maybe I'm misunderstanding?)

@silverwind
Copy link
Contributor

silverwind commented Jul 27, 2018

Current master branch (with ESLint 5.0.0) has globals 11.7.0

Ah, you're right. I actually attempted to remove those globals earlier on master but it wasn't working for some weird reason.

@silverwind
Copy link
Contributor

Just re-tested the removals of the globals, still didn't work out despite having the globals in tools/node_modules/eslint/node_modules/globals/globals.json. Have to check it out another time, so should be LGTM.

@Trott
Copy link
Member

Trott commented Jul 27, 2018

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

rubber-stamp lgtm

@Trott
Copy link
Member

Trott commented Jul 27, 2018

That one macstadium OSX 12 machine just doesn't want to build...

Trying again: https://ci.nodejs.org/job/node-test-pull-request/16032/

@Trott
Copy link
Member

Trott commented Jul 27, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 28, 2018
PR-URL: nodejs#21817
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Jul 28, 2018

Landed in 8c6d1a0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants