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.

Sign up for GitHub

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

Jump to bottom

tools: consolidate ESLint dependencies #40995

Closed
wants to merge 4 commits into from
Closed

tools: consolidate ESLint dependencies #40995

wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 27, 2021

Instead of installing everything separately, put it all under ESLint and take advantage of deduplication.

Probably easiest to focus on the second/middle commit of the three.

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Nov 27, 2021
@Trott Trott mentioned this pull request Nov 27, 2021
Merged
lpinca
lpinca approved these changes Nov 28, 2021
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

tools/update-eslint.sh Outdated
@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/sh -x
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#!/bin/sh -x
#!/bin/sh

tools/update-eslint.sh Outdated
@@ -8,27 +8,30 @@
set -e
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
set -e
set -ex

BridgeAR
BridgeAR approved these changes Nov 28, 2021
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

RSLGTM

@Trott Trott force-pushed the eslint-dedup branch 2 times, most recently from f29d457 to 80b300e Compare November 28, 2021 21:54
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2021
@Trott Trott added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Nov 29, 2021
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 29, 2021
@Trott
tools: run ESLint update to minimize diff on subsequent update
45422db 
PR-URL: nodejs#40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
tools: update ESLint update script to consolidate dependencies
6fd0a60 
PR-URL: nodejs#40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
tools: consolidate ESLint dependencies
4c41fea 
PR-URL: nodejs#40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
tools: remove unneeded tool in update-eslint.sh
6d8ab76 
The tool to remove absolute paths from package.json files is no longer
necessary. It appears that npm no longer stores these paths, or at least
not in a way that causes the kind of churn we saw in the past.

PR-URL: nodejs#40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 29, 2021

Landed in 1998e83...6d8ab76

@Trott Trott closed this Nov 29, 2021
@Trott Trott deleted the eslint-dedup branch November 29, 2021 22:54
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
tools: run ESLint update to minimize diff on subsequent update
953baaa 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
tools: update ESLint update script to consolidate dependencies
0aa589b 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
tools: consolidate ESLint dependencies
aff22e4 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
tools: remove unneeded tool in update-eslint.sh
6f1e3c6 
The tool to remove absolute paths from package.json files is no longer
necessary. It appears that npm no longer stores these paths, or at least
not in a way that causes the kind of churn we saw in the past.

PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
tools: run ESLint update to minimize diff on subsequent update
8427099 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
tools: update ESLint update script to consolidate dependencies
86d5af1 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
tools: consolidate ESLint dependencies
5400b79 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
tools: remove unneeded tool in update-eslint.sh
6525226 
The tool to remove absolute paths from package.json files is no longer
necessary. It appears that npm no longer stores these paths, or at least
not in a way that causes the kind of churn we saw in the past.

PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
@danielleadams
Copy link
Member

@Trott Looks like this one will be dependent on #41770, so marking as backport-blocked for now.

danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
tools: run ESLint update to minimize diff on subsequent update
0ed4775 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
tools: update ESLint update script to consolidate dependencies
83f9468 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
tools: consolidate ESLint dependencies
017cdcd 
PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
tools: remove unneeded tool in update-eslint.sh
e5cccdf 
The tool to remove absolute paths from package.json files is no longer
necessary. It appears that npm no longer stores these paths, or at least
not in a way that causes the kind of churn we saw in the past.

PR-URL: #40995
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Trott @danielleadams @jasnell @lpinca @tniessen @BridgeAR @nodejs-github-bot