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

Upgrade to node 20.12.22 & grunt-contrib-uglify to 5.2.2 #178

Closed

Conversation

Timodie
Copy link

@Timodie Timodie commented Aug 7, 2024

Description

In a similar fashion to silvermine/videojs-airplay#54, this PR upgrades the repo node 20.12.22 and also upgrades "grunt-contrib-uglify to 5.2.2

Tim Addai added 6 commits August 7, 2024 17:09
Updates the .nvmrc to point node version 20.12.2
Creates the necessary scripts to support
commands that will be exeucted during the build
The [email protected] package would break because Babel no
longer removes `const` based on our configurations and how they interact
with the latest browserslist output.

However, [email protected] or later does not break when it sees
`const`. So, we can update to the latest version, 5.2.2.
This was necessary due to the update to
the version of grunt-contrib-uglify.
Uglify was configured to mangle/compress in
debug mode and beautify mode in
production.This should be reversed.
The CI steps need to be updated to use Node 20.
While doing so, we incorporated some additional work:
- Update all actions to use the latest versions that run on Node 20
- Update the test matrix to use our .nvmrc file as one of the Node
versions to test againsts
- Check for uncommitted changes after installing dependencies
@Timodie Timodie changed the title Upgrade to node 20.12.22 Upgrade to node 20.12.22 & grunt-contrib-uglify to 5.2.2 Aug 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 18.386%. remained the same
when pulling 4cb0f48 on Timodie:taddai/upgrade-to-node-20.12.22
into 718c832 on silvermine:master.

Copy link

@kmuncie kmuncie left a comment

Choose a reason for hiding this comment

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

Just a first pass review, @joshuacurtiss can take it from here

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
Copy link

Choose a reason for hiding this comment

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

Should this be on a new line below?

name: Put NVM version in output
id: makeNodeVersionOutput
run: echo "nvmrc=$(cat .nvmrc)" >> "$GITHUB_OUTPUT"

Copy link

Choose a reason for hiding this comment

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

is this new line intended?

with:
node-version: ${{ matrix.node-version }}
- run: npm i -g [email protected]
- run: npm ci # Reinstall the dependencies to ensure they install with the current version of node
- run: npm test
Copy link

Choose a reason for hiding this comment

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

Should we be running the following before npm test?

         - run: npm run standards
         - run: npm run build # Ensure building is possible with this version of node

@@ -1 +1 @@
16.15.0
20.12.2
Copy link

Choose a reason for hiding this comment

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

I believe we are standardizing on including the v here

@@ -5,15 +5,18 @@
"main": "src/js/index.js",
"scripts": {
"commitlint": "commitlint --from 5ed6165",
"test": "check-node-version --npm 8.5.5 && nyc mocha -- -R spec 'tests/**/*.test.js'",
"check-node-version": "check-node-version --node $(cat .nvmrc) --npm 10.5.0 --print",
"test": "nyc mocha -- -R spec 'tests/**/*.test.js'",
Copy link

Choose a reason for hiding this comment

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

I don't see the -R "reporter" option configured in many other projects. I know it was here before, but should it continue to be included?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like -R spec is default behavior for this and future versions of mocha, so it is safe to omit.

@joshuacurtiss
Copy link
Contributor

I concur with @kmuncie, and may I add a few more points?

  • Is it ok if we change the PR title and description to say node 20.12.2? Just to reduce confusion for anyone that looks back. Unfortunately the branch name can't be changed without complications to the PR, I think.
  • Is it ok if we have the commits that upgrade packages be done with actual npm install, which will accordingly update package-lock.json and help the commit be a complete piece of work?
  • Whereas regenerating package-lock.json leaves us with the same result when we're all said and done, we regenerate package-lock.json separately because it's actually a different task with its own purpose (to get newest versions of all subdependencies), which also has its own handful of potential to introduce bugs. That's why I've found it meaningful to separate package-lock.json regeneration after doing specific package version upgrades.
  • Just a question: On the grunt-contrib-uglify commit, did you encounter the bug mentioned in the commit description, or were you copying the message from my commit in the other repo? Just checking. If you did encounter the same error, great! The message is clear. If not, upgrading the package is perfect, but let's not talk about an error that isn't happening in this repo, if that makes sense..
  • I just noticed that some of the commit message were really short line lengths. It's a minor thing but is it ok if we write out the messages in full lines?

@joshuacurtiss
Copy link
Contributor

Please reference #179.

@onebytegone
Copy link
Contributor

Superseded by #179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants