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: Update eslint, prettier, stylelint and dependencies #5782

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

anselmbradford
Copy link

@anselmbradford anselmbradford commented Feb 21, 2024

Summary

  • Update eslint, prettier, and stylelint, and bump dependencies.
  • Move stylelint config to an ESM module.
  • Update CSS formatting changes flagged by the linter.

Dependency updates

Dependency name Previous version New version
@18f/identity-stylelint-config 2.0.0 4.0.0
eslint 8.52.0 8.56.0
eslint-plugin-import 2.29.0 2.29.1
prettier 2.8.8 3.2.5
stylelint 15.11.0 16.5.0

@mejiaj mejiaj added this to the uswds 3.9.0 milestone Mar 8, 2024
@mejiaj
Copy link
Contributor

mejiaj commented Mar 8, 2024

Thanks for submitting this!

Adding to 3.9.0 milestone and noting that eslint-config-prettier was updated in 9054094.

@mejiaj mejiaj added the Status: Triage We're triaging this issue and grooming if necessary label Apr 4, 2024
@mejiaj mejiaj modified the milestones: uswds 3.9.0, uswds 3.8.1 Apr 4, 2024
@mejiaj mejiaj removed the Status: Triage We're triaging this issue and grooming if necessary label Apr 4, 2024
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.

Thanks for your work here @anselmbradford! Do you have any availability to pull the latest from USWDS devlop and resolve the merge conflicts?

Additionally, I see the following message when running the lint:sass script:

`output` is deprecated. Use `report` or `code` instead.

It looks like this could be resolved by replacing output with report on lintSass.js

uswds/tasks/lint.js

Lines 42 to 53 in 98566ec

async function lintSass(callback) {
const { errored, output } = await stylelint.lint({
files: [
`${PROJECT_SASS_SRC}/**/*.scss`,
`!${PROJECT_SASS_SRC}/uswds/**/*.scss`,
`!${PROJECT_SASS_SRC}/uswds-elements/lib/**/*.scss`,
],
formatter: "string",
});
callback(errored ? new Error(output) : null);
}

If you could make those changes it'd help us get this into the design system quicker! But if not, let us know and we'll create a new PR with your contribution 👍

@mahoneycm
Copy link
Contributor

Adding trailing commas to all multi-line statements

The new version of prettier updates the trailingCommas default to all. As you can see from the changed files, this adds additional commas to about 83 pages.

This blog post is a quick and easy read that outlines the benefit of adding these trailing commas. After reading it, I'm lean towards keeping the new standard for our code styles.

If we decide we don't want to keep the trailing commas for all lines, we can create a .prettierconfig file or use a flag to set the trailingCommas value to es5

@anselmbradford
Copy link
Author

@mahoneycm I'll aim to update this soon. May not be able to get to it today though.

@mahoneycm
Copy link
Contributor

@anselmbradford no immediate rush! I appreciate your work and the response 👍

@anselmbradford
Copy link
Author

anselmbradford commented Apr 24, 2024

@mahoneycm I've updated this. There seems to be a circle ci failure that looks unrelated to this PR?

Screenshot 2024-04-24 at 5 01 09 PM

EDIT: actually, I regenerated the package-lock.json and there are changes, so let's see how 8f0d5b2 fares.

EDIT2: Hmm, nope didn't help. Do you see what I am missing?

@mahoneycm
Copy link
Contributor

Hey there @anselmbradford,

This was tricky but it looks like it can be resolved by installing sass-embedded's optionalDependencies

uswds/package-lock.json

Lines 25373 to 25382 in 519108c

"optionalDependencies": {
"sass-embedded-darwin-arm64": "1.69.5",
"sass-embedded-darwin-x64": "1.69.5",
"sass-embedded-linux-arm": "1.69.5",
"sass-embedded-linux-arm64": "1.69.5",
"sass-embedded-linux-ia32": "1.69.5",
"sass-embedded-linux-x64": "1.69.5",
"sass-embedded-win32-ia32": "1.69.5",
"sass-embedded-win32-x64": "1.69.5"
}

I was able to confirm that we have the sass-embedded-darwin-arm64 directory in our node_modules on the develop branch, which is missing on this branch.

You can add it to your package-lock.json without needing to define it in package.json by running:

 npm install sass-embedded --save-exact --include=dev    

After this, I was able to install, build, and start the project without error 👍

@anselmbradford
Copy link
Author

@mahoneycm Thanks! Updated the PR (and also ran some Prettier fixes I missed)

@anselmbradford anselmbradford changed the title Update linters Update eslint, prettier, stylelint and dependencies Apr 25, 2024
@amyleadem amyleadem changed the title Update eslint, prettier, stylelint and dependencies USWDS - Update eslint, prettier, stylelint and dependencies Apr 29, 2024
@mahoneycm
Copy link
Contributor

Thanks for these updates @anselmbradford ! Planning to discuss our position on the trailing commas on all lines today and will be able to update this PR with our decision by EOD

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Question on the decision behind the stylelint config change, otherwise LGTM.

stylelint.config.mjs Outdated Show resolved Hide resolved
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! Thanks for your effort here and the quick respones @anselmbradford

Testing checklist

  • npm install runs without error
  • Linters run without error
  • Prettier runs without error
  • New trailingComma Prettier default adds value to code styles

Note

Stylelint CommonJS Node.js API is deprecated
This isn't an issue for this version of Stylelint (16.5.0) but will prevent us from updating to the next major version unless we migrate to ESM. 1

Footnotes

  1. Stylint: Deprecated CommonJS API

@mahoneycm mahoneycm requested a review from mejiaj May 7, 2024 14:36
@mahoneycm mahoneycm requested a review from amyleadem May 7, 2024 14:36
@mahoneycm mahoneycm changed the title USWDS - Update eslint, prettier, stylelint and dependencies USWDS - Dependencies: Update eslint, prettier, stylelint and dependencies May 7, 2024
@anselmbradford
Copy link
Author

Thanks @mahoneycm - What happens now, in terms of getting this merged?

@mahoneycm
Copy link
Contributor

@anselmbradford We'll get one more peer review and then move it onto our Final Fed review to be included in the upcoming release.

We have this slated for 3.8.1 which we're in the process of finalizing so you should see additional updates soon 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants