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 rollup to 2.58.0 #40281

Merged
merged 1 commit into from Oct 5, 2021
Merged

tools: update rollup to 2.58.0 #40281

merged 1 commit into from Oct 5, 2021

Conversation

iam-frankqiu
Copy link
Contributor

No description provided.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 1, 2021
aduh95
aduh95 previously approved these changes Oct 2, 2021
@aduh95 aduh95 changed the title tools: update rollup to 2.5.8 tools: update rollup to 2.58.0 Oct 2, 2021
@iam-frankqiu iam-frankqiu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2021
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.

There is a typo in commit message 2.5.8 -> 2.58.0

@Trott
Copy link
Member

Trott commented Oct 3, 2021

I imagine this is obsoleted by 8a58fce. If not, be sure to run make lint-md-rollup and commit any changes from that task.

@iam-frankqiu
Copy link
Contributor Author

I imagine this is obsoleted by 8a58fce. If not, be sure to run make lint-md-rollup and commit any changes from that task.

Actually, it's not. I have already run make lint-md-rollup and update the last code from that task.

@Trott
Copy link
Member

Trott commented Oct 3, 2021

Actually, it's not.

What makes you say that it isn't?

Here's why I think it is. We're already using rollup 2.58.0 on the master branch:

$ git checkout master
Already on 'master'
Your branch is up-to-date with 'origin/master'.
$ cd tools/lint-md
$ npm ci

added 184 packages, and audited 185 packages in 2s

133 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
$ npm ls rollup
lint-md@1.0.0 /Users/trott/io.js/tools/lint-md
├─┬ @rollup/plugin-commonjs@20.0.0
│ ├─┬ @rollup/pluginutils@3.1.0
│ │ └── rollup@2.58.0 deduped
│ └── rollup@2.58.0 deduped
├─┬ @rollup/plugin-node-resolve@13.0.5
│ └── rollup@2.58.0 deduped
└── rollup@2.58.0

$

@Trott
Copy link
Member

Trott commented Oct 3, 2021

Not sure why it's being weird in the GitHub interface, but the diff against master just shows that the package.json (and the copy of package.json in package-lock.json) are updated but none of the actual package versions in package-lock.json are updated:

diff --git a/tools/lint-md/package-lock.json b/tools/lint-md/package-lock.json
index 14ac153c4c..dfc8f3194b 100644
--- a/tools/lint-md/package-lock.json
+++ b/tools/lint-md/package-lock.json
@@ -18,7 +18,7 @@
       "devDependencies": {
         "@rollup/plugin-commonjs": "^20.0.0",
         "@rollup/plugin-node-resolve": "^13.0.5",
-        "rollup": "^2.57.0"
+        "rollup": "^2.58.0"
       }
     },
     "node_modules/@rollup/plugin-commonjs": {
diff --git a/tools/lint-md/package.json b/tools/lint-md/package.json
index 60c68f41ee..ffa82b4b14 100644
--- a/tools/lint-md/package.json
+++ b/tools/lint-md/package.json
@@ -16,6 +16,6 @@
   "devDependencies": {
     "@rollup/plugin-commonjs": "^20.0.0",
     "@rollup/plugin-node-resolve": "^13.0.5",
-    "rollup": "^2.57.0"
+    "rollup": "^2.58.0"
   }
 }

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

This change is a no-op as far as I can tell. See previous comments.

@Trott
Copy link
Member

Trott commented Oct 3, 2021

(To see the diff above, make sure upstream/master is up-to-date, check out this branch and do git rebase upstream/master.)

@iam-frankqiu iam-frankqiu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 4, 2021
@iam-frankqiu
Copy link
Contributor Author

--- a/tools/lint-md/package.json

That's why I said above. It's also pretty confused me.

Trott
Trott previously requested changes Oct 4, 2021
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

--- a/tools/lint-md/package.json

That's why I said above. It's also pretty confused me.

I can see why it's confusing.

The package.json update has no effect. On current master branch, it lists rollup ^2.57.0 which means 2.x where x is at least 57. The package-lock.json file that's there guarantees that 2.58.0 is installed. The make lint-md-rollup process then uses rollup 2.58.0 to create tools/lint-md/lint-md.js. This is all on current master branch.

All this change does is update the package.json but it has no effect on any code or on the installation of any dependencies including rollup. (See above. We're already using 2.58.0 on master.) This doesn't "update" rollup. It just changes the package.json (and the copy of the package.json that is in package-lock.json.) Those changes have no effect.

Again, I can understand why this is confusing. The creation of the rollup-generated linter is not a matter of simply using npm to update dependencies in tools/lint-md and I don't want to start landing changes that suggest it is.

@aduh95 aduh95 dismissed their stale review October 4, 2021 16:12

It looks like Rich is correct.

@DerekNonGeneric
Copy link
Contributor

I don't see the point in blocking this PR… We should always keep our versions up-to-date in the package.json — regardless of whether or not it is a no-op.

@Trott
Copy link
Member

Trott commented Oct 5, 2021

We should always keep our versions up-to-date in the package.json — regardless of whether or not it is a no-op.

I'm not sure I agree with that, but I'm not sure I disagree either, and I know I don't want to have to think about it. So sure, let's land it. The commit message should be revised to accurately reflect the change, since this doesn't (anymore) update rollup, so I'll do that while landing it now.

@Trott Trott dismissed their stale review October 5, 2021 05:34

sure why not

Trott pushed a commit to iam-frankqiu/node that referenced this pull request Oct 5, 2021
Update package.json to indicate that rollup 2.58.0 is the minimum. That
version was already installed by a previous commit.

PR-URL: nodejs#40281
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 5, 2021

Landed in dd3c4a5

Update package.json to indicate that rollup 2.58.0 is the minimum. That
version was already installed by a previous commit.

PR-URL: nodejs#40281
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott merged commit dd3c4a5 into nodejs:master Oct 5, 2021
@targos
Copy link
Member

targos commented Oct 5, 2021

We should always keep our versions up-to-date in the package.json — regardless of whether or not it is a no-op.

I'm not sure I agree with that, but I'm not sure I disagree either, and I know I don't want to have to think about it. So sure, let's land it. The commit message should be revised to accurately reflect the change, since this doesn't (anymore) update rollup, so I'll do that while landing it now.

To avoid this, I think we should always update both package.json and package-lock.json when we bump dependencies.

danielleadams pushed a commit that referenced this pull request Oct 5, 2021
Update package.json to indicate that rollup 2.58.0 is the minimum. That
version was already installed by a previous commit.

PR-URL: #40281
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.

None yet

10 participants