Upgrade to node 20.12.2 #111
Conversation
Updates the .nvmrc to point node version 20.12.2
Creates the necessary scripts to support commands that will be exeucted during the build
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
There was a problem hiding this comment.
Just a first pass review, @joshuacurtiss can take it from here
| - run: npm run standards | ||
| - run: npm run build |
There was a problem hiding this comment.
Is this missing the "Check for uncommitted changes" on purpose?
| name: PUT NVM version in output | ||
| id: makeNodeVersionOutput | ||
| run: echo "nvmrc=$(cat .nvmrc)" >> "$GITHUB_OUTPUT" | ||
|
|
| test: | ||
| needs: [ build ] | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| node-version: [ 14, 16, 'lts/*', 'latest' ] | ||
| node-version: [ 16,'${{ needs.build.outputs.nvmrc }}', 'lts/*', 'latest' ] |
| 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 |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
I believe we are standardizing on including the v here
| @@ -91,9 +91,9 @@ module.exports = function(grunt) { | |||
| banner: '/*! <%= pkg.name %> <%= grunt.template.today("yyyy-mm-dd") %> <%= versionInfo %> */\n', | |||
| sourceMap: true, | |||
There was a problem hiding this comment.
I believe we also usually use DEBUG on this line and the following
| @@ -4,8 +4,11 @@ | |||
| "description": "video.js plugin for selecting a video quality or resolution", | |||
| "main": "src/js/index.js", | |||
| "scripts": { | |||
| "prepare": "grunt build", | |||
| "test": "check-node-version --npm 8.5.5 && nyc mocha -- 'tests/**/*.test.js'", | |||
| "prepare": "npm run build", | |||
There was a problem hiding this comment.
The name prepare seems out of norm compared to our other projects. Should this be changed to prepublish?
| "test": "check-node-version --npm 8.5.5 && nyc mocha -- 'tests/**/*.test.js'", | ||
| "prepare": "npm run build", | ||
| "check-node-version": "check-node-version --node $(cat .nvmrc) --npm 10.5.0 --print", | ||
| "test": "nyc mocha -- -R spec 'tests/**/*.test.js'", |
There was a problem hiding this comment.
I don't see the -R "reporter" option configured in many other projects, is that intended to be added here where it wasnt present before?
|
Please reference #112. |
|
Superseded by #112 |
Description
Modeled after silvermine/videojs-airplay#54
This PR upgrades this project to node 20.12.2 but does not update the grunt-contrib-uglify version