Update stylelint config

[anselmbradford]

• The latest stylelint has deprecated commonjs and moved to ESM https://stylelint.io/migration-guide/to-16/
  It looks like the config for @18f/identity-stylelint-config should be updated to ESM https://github.com/18F/identity-idp/blob/main/app/javascript/packages/stylelint-config/index.js#L1

• Could we add the { resolveNestedSelectors: true } option to selector-class-pattern to lint nested class names inside SCSS? See https://stylelint.io/user-guide/rules/selector-class-pattern/#optional-secondary-options

• It seems like stylelint-config-standard-scss should replace stylelint-config-recommended-scss, as it extends both stylelint-config-standard and stylelint-config-recommended-scss (see example docs at https://stylelint.io/user-guide/get-started/#linting-css-like-languages-and-css-within-containers)

• Use of stylelint-prettier is not recommended, according to https://prettier.io/docs/en/integrating-with-linters.html#notes

[aduth]

@anselmbradford Thanks for the pointers and feedback! I'll plan to review these soon, and will follow-up by end-of-day tomorrow.

[aduth]

Some initial impressions before I look to make some changes here based on your feedback. Let me know what you think!

• The latest stylelint has deprecated commonjs and moved to ESM https://stylelint.io/migration-guide/to-16/
  It looks like the config for @18f/identity-stylelint-config should be updated to ESM https://github.com/18F/identity-idp/blob/main/app/javascript/packages/stylelint-config/index.js#L1

I think we may hold off on this until the point where Stylelint goes ESM-only, or at least we make the decision to do so for shared configuration. This would at least require that we get this project itself on ESM, which it's not currently. As it stands, I don't think Stylelint's switch to ESM has a meaningful impact on most downstream projects, and ESM projects can still load the configuration as CommonJS.

The reason I wouldn't want to do a "hybrid" package like Stylelint is that it'll require a pre-publish build step to compile separate ESM and CJS versions, which adds a lot of unnecessary complexity.

• Could we add the { resolveNestedSelectors: true } option to selector-class-pattern to lint nested class names inside SCSS? See https://stylelint.io/user-guide/rules/selector-class-pattern/#optional-secondary-options

I think that makes a lot of sense! I'll look to enable that option..

• It seems like stylelint-config-standard-scss should replace stylelint-config-recommended-scss, as it extends both stylelint-config-standard and stylelint-config-recommended-scss (see example docs at https://stylelint.io/user-guide/get-started/#linting-css-like-languages-and-css-within-containers)

I think you're right, but this also feels like a substantial change that is quite likely to flag new, previously-unflagged code, since it's enforcing a lot more rules. I think we'll want to save this until at least the next major release, which with Stylelint major releases probably won't be too long 😅

• Use of stylelint-prettier is not recommended, according to https://prettier.io/docs/en/integrating-with-linters.html#notes

I think I generally agree with this, though the Prettier documentation is written with the assumption that a project is using Prettier, where the original inclusion of stylelint-prettier was to allow the configuration to work easily out-of-the-box if someone was using Stylelint, but maybe not Prettier.

But since we require Prettier as a peer dependency anyways, it might be better to encourage projects to lint Sass formatting using Prettier. At the very least, I think it should be possible to easily disable the plugin if someone did want to do that.

[anselmbradford]

I think we may hold off on this until the point where Stylelint goes ESM-only, or at least we make the decision to do so for shared configuration. This would at least require that we get this project itself on ESM, which it's not currently. As it stands, I don't think Stylelint's switch to ESM has a meaningful impact on most downstream projects, and ESM projects can still load the configuration as CommonJS.

One thing to perhaps test is when downstream projects have "type": "module" set in their package.json, they may expect files named index.js to be ESM, and commonjs files to be named index.cjs. Off the top of my head, I think that may only be an issue if the project were to copy this project's configuration into their project. With the config coming in through an external dependency and living in node_modules I think it will be okay, but I'm not totally certain off the top of my head.

[aduth]

Yeah, the way we currently assign exports would prevent people from being able to reference the file directly from within the package. In other words, only import '@18f/identity-stylelint-config'; is supported, not import '@18f/identity-stylelint-config/index.js';, so they shouldn't be interfacing directly with individual files from the package.

I wouldn't expect it to be very common that someone copies the source .js verbatim into their own project. If they did, I think it's reasonable to expect it to be their responsibility to determine whether their project's .js defaults to ESM or CommonJS, and either adapt the code or rename the file accordingly.

[aduth]

I spent some time trying out replacing stylelint-config-recommended-scss with stylelint-config-standard-scss in this project. It caught a lot of new issues (97 errors) which would likely create a burden for downstream projects to migrate, but in general I agree with most of the additional new rules. I think there's a few I'd probably opt to disable, such as those which prevent blank lines where I think it's reasonable to allow blank lines as a form of organizing code (rule-empty-line-before, scss/dollar-variable-empty-line-before). There's also some like selector-not-notation which may be non-trivial to work-around.

[aduth]

Also, I think migrating to the standard ruleset is probably a good idea to bring this configuration closer in line with the TTS Engineering guidelines, which also recommend this ruleset:

https://guides.18f.gov/engineering/languages-runtimes/css/#setting-up-stylelint-locally

[anselmbradford]

Yeah I'd probably update to the standard config and turn off any rules you're concerned might be too much at this stage, then make a release with the turned off rules, and then on the next Stylelint major release start removing disabled rules.

[aduth]

Good idea. Maybe we can configure all the rules as warning severity in a minor release as a deprecation path, and then plan to switch them to error severity in the next major release.

[aduth]

After spending a little time with trying to create a deprecation path, I realized it's going to be a substantial effort to list and override each additional rule to be downgraded to a warning.

Instead, here's what I'm planning:

1. Add { resolveNestedSelectors: true } option to selector-class-pattern and publish in a minor release (4.1.0)
2. Replace stylelint-config-recommended-scss with stylelint-config-standard-scss and publish in a beta major release (5.0.0-beta.1)
3. The final major release will be published along with the next round of Stylelint major version updates

[aduth]

1. Add { resolveNestedSelectors: true } option to selector-class-pattern and publish in a minor release (4.1.0)
2. Replace stylelint-config-recommended-scss with stylelint-config-standard-scss and publish in a beta major release (5.0.0-beta.1)

These are now both available on NPM!

https://www.npmjs.com/package/@18f/identity-stylelint-config?activeTab=versions

You should be able to install 5.0.0-beta.1 if you want to use all of these changes.

I'll plan to close this issue as completed, with a couple caveats:

• This isn't entirely completed until the final 5.0.0 release, but that'll be handled as part of the next Stylelint major update
  • I know this is something we've missed in the past (Update dependencies in @18f/identity-stylelint-config to support stylelint v16 #10124), so I might see if I can configure our dependabot.yml to auto-update on the major version, so we can keep the configuration in lock-step.
• For the time being, I think I want to keep stylelint-prettier as-is. If someone wants to opt-out of this behavior and run Prettier directly instead, it should be as simple as extending the rules to disable the Prettier rule:

{
  "extends": "@18f/identity-stylelint-config",
  "rules": {
    "prettier/prettier": null
  }
}

Thanks again for the continued feedback and ideas, @anselmbradford !