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

USWDS - Dependencies: Use node LTS 20.9.0 #5560

Merged
merged 9 commits into from Oct 30, 2023

Conversation

mejiaj
Copy link
Contributor

@mejiaj mejiaj commented Oct 12, 2023

Summary

Set node 18 as the default. Updated .nvmrc file from 16→20, which is the current LTS version.

Breaking change

This is not a breaking change.

Related issue

Closes #5010.

Related pull requests

Related to uswds/uswds-site#2304.

Preview link

Preview link:

Problem statement

Support for node 16 is going to stop on CloudGov pages.

Solution

Note
Storybook v7 is a breaking change and will be done in #5239.

  1. Update .nvmrc to 20.
  2. Update .tool-versions to npmjs 20.9.0.
  3. Update StorybookJS to latest 6x release based on this chart [storybook.js.org].
  4. Update node dependencies to latest non-breaking version.
  5. Test StorybookJS & gulp scripts.

Major changes

  • Updated node LTS version in NVMRC from 16→20
  • Updated .tool-versions to npmjs 20.9.0.
  • Updated StorybookJS to last 6x version.
  • Updated NPM dependencies.

Testing and review

  1. Run npm install.
  2. Run npm start to run StorybookJS without errors.
  3. Visit icons page to confirm icons are displaying as expected.
  4. Run npx gulp build to confirm builds.

Checklist

  • No regressions in icons.
  • No regressions in SASS compilation.
  • No regressions in linting & prettier.
  • No regressions in html:build.
  • No regressions in StorybookJS.
  • No regressions in terminal logs (doc-utils.js - ex: npx gulp build).

Dependency updates

Dependency name Previous version New version
classlist-polyfill 1.0.3 1.2.0
@babel/core 7.16.7 7.23.2
@babel/preset-env 7.16.7 7.23.2
@chanzuckerberg/axe-storybook-testing 6.3.0 6.3.1
@material-design-icons/svg 0.11.1 0.14.13
@storybook/addon-a11y 6.5.12 6.5.16
@storybook/addon-essentials 6.5.12 6.5.16
@storybook/addon-links 6.5.12 6.5.16
@storybook/builder-webpack5 6.5.12 6.5.16
@storybook/html 6.5.12 6.5.16
@storybook/manager-webpack5 6.5.12 6.5.16
@types/node 16.11.19 20.8.9
ansi-colors 4.1.1 4.1.3
autoprefixer 10.4.1 10.4.16
axe-core 4.6.3 4.8.2
babel-loader 8.2.2 9.1.3
css-loader 6.2.0 6.8.1
eslint 8.4.1 8.52.0
eslint-config-prettier 8.3.0 9.0.0
eslint-plugin-import 2.25.4 2.29.0
eslint-plugin-no-unsanitized 4.0.1 4.0.2
fancy-log 1.3.3 2.0.0
gulp-replace 1.1.3 1.1.4
handlebars-helpers 0.9.8 0.10.0
html-webpack-plugin 5.4.0 5.5.3
postcss-csso 6.0.0 6.0.1
postcss-discard-comments 5.0.1 6.0.0
postcss-loader 6.1.1 7.3.3
postcss-preset-env 7.4.2 9.2.0
prettier 2.4.1 2.8.8
sass-embedded 1.58.3 1.69.5
sass-loader 12.1.0 13.3.2
snyk 1.1064.0 1.1237.0
style-loader 3.3.0 3.3.3
stylelint 15.10.1 15.11.0
svgo 2.8.0 3.0.2
twigjs-loader 1.0.2 1.0.3
typescript 4.4.4 5.2.2
webpack 5.76.0 5.89.0
webpack-cli 4.9.1 5.1.4

package.json Outdated
"postcss-sass-loader": "1.1.0",
"prettier": "2.4.1",
"prettier": "3.0.3",
Copy link
Contributor Author

@mejiaj mejiaj Oct 12, 2023

Choose a reason for hiding this comment

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

Prettier 3x

Tried prettier v3.0.3, but reverting to last 2x version [2.8.8] because of conflicts with other prettier plugins.

Prettier 3x error

➜ npx gulp lintSass

[15:37:11] Using gulpfile ~/web/uswds/gulpfile.js
[15:37:11] Starting 'lintSass'...
[15:37:13] 'lintSass' errored after 2.02 s
[15:37:13] TypeError: prettier.resolveConfig.sync is not a function
    at /Users/jmejia-a/web/uswds/node_modules/stylelint-prettier/stylelint-prettier.js:50:56
    at /Users/jmejia-a/web/uswds/node_modules/stylelint/lib/lintPostcssResult.js:121:8
    at Array.map (<anonymous>)
    at lintPostcssResult (/Users/jmejia-a/web/uswds/node_modules/stylelint/lib/lintPostcssResult.js:111:18)
    at lintSource (/Users/jmejia-a/web/uswds/node_modules/stylelint/lib/lintSource.js:110:8)
    at async /Users/jmejia-a/web/uswds/node_modules/stylelint/lib/standalone.js:211:27
    at async Promise.all (index 8)
    at async Function.standalone [as lint] (/Users/jmejia-a/web/uswds/node_modules/stylelint/lib/standalone.js:254:22)
    at async lintSass (/Users/jmejia-a/web/uswds/tasks/lint.js:43:31)

See all prettier releases here [GitHub].

Comment on lines +118 to +123
"@storybook/addon-a11y": "6.5.16",
"@storybook/addon-essentials": "6.5.16",
"@storybook/addon-links": "6.5.16",
"@storybook/builder-webpack5": "6.5.16",
"@storybook/html": "6.5.16",
"@storybook/manager-webpack5": "6.5.16",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Latest 6x StorybookJS version. The next major version, 7 requires major updates. Captured in #5239.

@mejiaj mejiaj marked this pull request as ready for review October 13, 2023 16:10
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Terminal message [OUTDATED]

Previous review

This is looking good to me! Just had one potential regression in the terminal logs.

When I open the console in storybook I see a collapsable warning with a sass-loader error followed by our USWDS Notifications:

image

I'm not seeing the same thing on our current build.

Testing Checklist

  • No regressions in icons.
  • No regressions in SASS compilation.
  • No regressions in linting & prettier.
  • No regressions in html:build.
  • No regressions in StorybookJS.
  • No regressions in terminal logs (doc-utils.js - ex: npx gulp build).
  • Confirmed identity-stylelint-config@2.0.0 is the latest

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting this PR resolves 1 critical vulnerability and introduces 3 moderate vulnerabilities.

Develop:

50  vulnerabilities (1 low, 21 moderate, 27 high, 1 critical)

This branch:

51 vulnerabilities (24 moderate, 27 high)

Running npm audit fix didn't resolve any of the new vulnerabilities.

@mahoneycm
Copy link
Contributor

Scratch that console message note. I see it on other branches as well as develop now.

@mejiaj
Copy link
Contributor Author

mejiaj commented Oct 23, 2023

@mahoneycm would you mind updating the comment with the outdated console message note? It could be potentially confusing to new readers.

Maybe hide it in a <details> element labeled Previous note or something marking it as outdated.


Also, do you have any requested changes or should I request another re-review from you?

@mejiaj mejiaj added this to the uswds 3.7.0 milestone Oct 23, 2023
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Lgtm!

The previous regression I flagged turned out to be a false alarm.

Testing Checklist

  • No regressions in icons.
  • No regressions in SASS compilation.
  • No regressions in linting & prettier.
  • No regressions in html:build.
  • No regressions in StorybookJS.
  • No regressions in terminal logs (doc-utils.js - ex: npx gulp build).
  • Confirmed identity-stylelint-config@2.0.0 is the latest

@mejiaj mejiaj requested a review from amyleadem October 24, 2023 14:54
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

This looks good to me! I was able to successfully perform the following tests using both Node 16.18.0 and 18.12.0:

  • Confirmed I can successfully run the following:
    • npm install
    • npm run start
    • npm run build
    • npm run build:html
    • npm run prettier
    • npm run lint
  • Confirmed that Storybook builds and displays as expected
  • Confirmed icons display as expected

I did notice that there have been some minor and patch updates in the dependencies since this PR was opened. I tagged them in the comments for your awareness in case we want to update.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
thisisdano and others added 3 commits October 30, 2023 15:18
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
@thisisdano
Copy link
Member

Note that Run npx gulp build to confirm builds. does not work since build is not a current task. But Run npx gulp does work as expected

@thisisdano
Copy link
Member

I'd like to consider modifying this PR to move to the current Node LTS, 20.9.0. I've tested it on this PR and it seems to work.

@thisisdano
Copy link
Member

I was also able to successfully perform the following tests using Node 20.9.0:

  • Confirmed I can successfully run the following:
    • npm install
    • npm run start
    • npm run build
    • npm run build:html
    • npm run prettier
    • npm run lint
  • Confirmed that Storybook builds and displays as expected
  • Confirmed icons display as expected

I'm going to test a bit on uswds-site and if it works as expected, I'll probably update this PR.

@thisisdano thisisdano changed the title USWDS - Dependencies: Use node LTS 18 USWDS - Dependencies: Use node LTS 20.9.0 Oct 30, 2023
@thisisdano
Copy link
Member

thisisdano commented Oct 30, 2023

OK, done. Updated description as well as the dependency table

@@ -6,7 +6,7 @@ orbs:
references:
container: &container
docker:
- image: cimg/node:16.15.0-browsers
- image: cimg/node:20.9.0-browsers
Copy link
Member

Choose a reason for hiding this comment

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

Updated Docker image

@thisisdano thisisdano merged commit c5b94aa into develop Oct 30, 2023
5 checks passed
@thisisdano thisisdano deleted the jm-feature-node-18-update branch October 30, 2023 23:10
@thisisdano thisisdano mentioned this pull request Nov 9, 2023
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.

Update to latest Node LTS
4 participants