chore: update stylelint config settings #2406

castastrophe · 2024-01-05T17:25:30Z

Description

This PR updates the stylelint settings and adds a listing task to the build.

New stylelint plugins:

stylelint-no-missing-var: ensures custom properties are not missing the var() function wrapper.
stylelint-no-unknown-custom-properties: reports on custom properties that cannot be found locally or in the token package.
stylelint-no-unused-custom-properties: reports on custom properties defined in a package but not used by the component.

Removes stylelint-suit-naming-pattern in favor of the existing selector-bem-pattern plugin:

"plugin/selector-bem-pattern": [
  {
    preset: "suit",
    presetOptions: { namespace: "spectrum" },
    utilitySelectors: /^\.(is|u)-[A-z0-9]+$/,
    componentName: /^[A-Z][A-z0-9]+$/,
  },
  {
    severity: "warning",
  },
],

How and where has this been tested?

yarn lint: runs and reports linting errors to the console
yarn linter accordion: runs linter against accordion and reports errors to the console
yarn linter accordion --fix: runs linter and fixes what it can in the pre-compiled assets in the accordion package. Note do not commit these updated assets at this time (at least not as part of this PR).
Expect none of the above commands to block commits or merges if there are linting errors.
yarn test:plugins: this new command runs the existing test suite for the stylelint plugins and reports their success (or failure).

Tested by @jawinn -- note: --verbose is needed to see warnings. I also saw that the automatic --fix can create duplicate declarations, though I'm assuming this is expected to not be perfect and won't be run without human intervention of the output before commit.

To-do list

I have read the contribution guidelines.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

github-actions · 2024-01-05T17:41:49Z

File metrics

Summary

Total size: 3.92 MB^*

🎉 No changes detected in any packages

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2024-01-05T17:42:15Z

🚀 Deployed on https://pr-2406--spectrum-css.netlify.app

jawinn

I was able to verify that the commands in the validation instructions are working, with --verbose needed to see the warnings in the output. I tested out the plugins by modifying CSS and running linter on Accordion:

Plugins testing

no-unknown-custom-properties - working
--spectrum-accordion-test-2: var(--spectrum-test-undefined);

stylelint-no-unused-custom-properties - not working?
--spectrum-test: red;
--spectrum-accordion-test: 10px;
I'm not seeing a warning for either of these added test properties.

~~stylelint-no-missing-parenthesis~~ stylelint-no-missing-var - working
--spectrum-accordion-item-width: --spectrum-accordion-minimum-width; (also tested w/ line break)

~~stylelint-no-missing-var~~ stylelint-no-missing-parenthesis - is this still needed?
I already get a regular syntax error from the lint command when testing out the missing bracket examples: "Unclosed bracket CssSyntaxError". Is this now covered by the linter by default? I wasn't able to create an example where I could see a warning from this plugin because of the syntax error being raised first.

jawinn · 2024-01-09T17:16:53Z

README.md

+- `yarn lint`: Provides helpful updates and warnings for a component's package.json file. This helps keep all components in alignment.
+  - Use `yarn lint --fix` to automatically fix any issues that are found.
+  - To run on a single component, use `yarn linter accordion` (where `accordion` is the name of the component or components you want to lint).


Could we add something about using the --verbose flag to show warnings? Currently the commands will only show errors. It might also be worth noting that --fix will try to fix both warnings and errors.

If possible, I'd like to add this as a backlog story as well so we can focus on the build rework and return to this for documentation.

I'll add the other reason I want to hold off documenting this is because it's more complex than just adding a verbose flag. You can use that or set the NX_VERBOSE_LOGGING=true variable in your .env or your ~/.env local file. I've also evaluated in build tooling updates forcing the verbose output which would not require a flag.

jawinn · 2024-01-09T17:57:33Z

stylelint.config.js

+          /^--spectrum-(global|alias|component)-/,
+          /^--spectrum-animation-/,


I'm curious if we could drop these two ignore settings. I tested removing them and found at least one usage of a "spectrum-global" variable that should not be used within a migrated component. It looks like global/alias/component are there to avoid warnings in non-migrated components that use those old tokens (I think the components being deprecated and the in-review Icon are the last remaining?).

When we remove the last @spectrum-css/vars package, I think that's the time to remove these entries rather than in advance. While I like the idea that we'd catch them in migrated components, it's a nice-to-have vs. throwing a false error on un-migrated assets.

jawinn · 2024-01-09T18:26:51Z

plugins/stylelint-no-unknown-custom-properties/index.js

+        const rootIdx = parts.indexOf("components");
+        const componentRoot = parts.slice(0, rootIdx + 2).join(path.sep);
+
+        function fetchResolutions(depName) {


Could you add a JSDoc style comment to this function, as well as some additional comments in this file for some of the larger blocks of code (e.g. line 73, 86, 107, etc)?

I can but can we make that a post-merge to-do since it's not affecting functionality? Only ask because this is a step toward simplifying the build migration and the build migration is my main focus. I can add a backlog story to ensure this gets returned to.

(Part of my hesitation to do it now is that I didn't write the plugin, it was written by Rajdeep for the build rework so to add a solid JS-doc, I'd need to read through the whole thing again and re-acquaint myself with it's inner workings)

castastrophe · 2024-01-10T14:23:09Z

stylelint-no-missing-var - is this still needed?
I already get a regular syntax error from the lint command when testing out the missing bracket examples: "Unclosed bracket CssSyntaxError". Is this now covered by the linter by default? I wasn't able to create an example where I could see a warning from this plugin because of the syntax error being raised first.

This isn't verifying missing brackets, but rather, missing var functions. Did you test adding a color: --test-variable to see if it would throw the error?

jawinn · 2024-01-10T15:25:43Z

stylelint-no-missing-var - is this still needed?
I already get a regular syntax error from the lint command when testing out the missing bracket examples: "Unclosed bracket CssSyntaxError". Is this now covered by the linter by default? I wasn't able to create an example where I could see a warning from this plugin because of the syntax error being raised first.

This isn't verifying missing brackets, but rather, missing var functions. Did you test adding a color: --test-variable to see if it would throw the error?

Sorry, that comment was actually meant for the "stylelint-no-missing-parenthesis". I've edited the original comment.

castastrophe · 2024-01-10T22:17:09Z

@jawinn Should be ready for a re-review

- feat: remove theme files without content

castastrophe · 2024-01-11T14:20:45Z

@jawinn Ah yes good call-out. This latest update includes removing stylelint-no-missing-parenthesis. I've also addressed the need to add verbose all the time by adding it to the yarn command and then we can go about documenting the different ways of controlling console output during the build rework.

jawinn · 2024-01-11T18:58:04Z

Looking good! I am seeing spectrum-tools/no-unused-custom-properties working now. And the added warning output with the lint and linter commands.

One last thing I saw when running yarn linter accordion:
"Invalid Option: Invalid option value "false" for rule "spectrum-tools/no-unknown-custom-properties". Are you trying to disable this rule? If so use "null" instead"
After that, the PR should be good to go.

Do we also want to disable the --fix running on commit in this PR? (main)

castastrophe · 2024-01-11T20:37:44Z

Looking good! I am seeing spectrum-tools/no-unused-custom-properties working now. And the added warning output with the lint and linter commands.

One last thing I saw when running yarn linter accordion: "Invalid Option: Invalid option value "false" for rule "spectrum-tools/no-unknown-custom-properties". Are you trying to disable this rule? If so use "null" instead" After that, the PR should be good to go.

Do we also want to disable the --fix running on commit in this PR? (main)

Genuinely no idea what's that referring to. If you look at the config, it's using null. I don't see any signs of false being used.

Update moved a few validation items around and that seems to make it a bit less angry. Was throwing a false error. Should be good now though!

castastrophe · 2024-01-11T21:04:27Z

@jawinn Oh and I pushed up a removal of stylelint from the lint-staged task list and just replaced it with a vanilla prettier.

castastrophe force-pushed the chore-stylelint-config-updates branch 2 times, most recently from 2c18ffb to 2b40bcb Compare January 5, 2024 17:32

castastrophe added skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review build Issues associated with the build process; often a refactor size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. labels Jan 5, 2024

castastrophe self-assigned this Jan 5, 2024

castastrophe force-pushed the chore-stylelint-config-updates branch 4 times, most recently from f44bc84 to 6bdda03 Compare January 5, 2024 21:06

castastrophe requested a review from pfulton January 8, 2024 15:36

castastrophe force-pushed the chore-stylelint-config-updates branch 4 times, most recently from 7cce861 to 3d6678c Compare January 8, 2024 16:49

jawinn self-requested a review January 9, 2024 16:49

jawinn requested changes Jan 9, 2024

View reviewed changes

castastrophe force-pushed the chore-stylelint-config-updates branch from 3d6678c to 1e413fa Compare January 10, 2024 13:30

castastrophe force-pushed the chore-stylelint-config-updates branch 4 times, most recently from 32538b3 to c7fe3a7 Compare January 10, 2024 22:04

build: fix spectrum copyright being stripped on build

b3d69af

- feat: remove theme files without content

castastrophe force-pushed the chore-stylelint-config-updates branch from c7fe3a7 to c1d8f20 Compare January 11, 2024 14:18

castastrophe requested a review from jawinn January 11, 2024 14:20

castastrophe force-pushed the main branch from b3d69af to 1fc9832 Compare January 11, 2024 20:11

castastrophe force-pushed the chore-stylelint-config-updates branch 3 times, most recently from 82e704b to 68b8b20 Compare January 11, 2024 21:01

chore: update stylelint config settings

4b9af79

castastrophe force-pushed the chore-stylelint-config-updates branch from 68b8b20 to 4b9af79 Compare January 12, 2024 15:46

jawinn approved these changes Jan 12, 2024

View reviewed changes

castastrophe merged commit c8800a2 into main Jan 12, 2024
10 checks passed

castastrophe deleted the chore-stylelint-config-updates branch January 12, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update stylelint config settings #2406

chore: update stylelint config settings #2406

castastrophe commented Jan 5, 2024 •

edited by jawinn

Loading

github-actions bot commented Jan 5, 2024 •

edited

Loading

github-actions bot commented Jan 5, 2024 •

edited

Loading

jawinn left a comment •

edited

Loading

jawinn Jan 9, 2024

castastrophe Jan 10, 2024

castastrophe Jan 10, 2024

jawinn Jan 9, 2024

castastrophe Jan 10, 2024

jawinn Jan 9, 2024

castastrophe Jan 10, 2024

castastrophe Jan 10, 2024

castastrophe commented Jan 10, 2024

jawinn commented Jan 10, 2024 •

edited

Loading

castastrophe commented Jan 10, 2024

castastrophe commented Jan 11, 2024

jawinn commented Jan 11, 2024 •

edited

Loading

castastrophe commented Jan 11, 2024 •

edited

Loading

castastrophe commented Jan 11, 2024

		/^--spectrum-(global\|alias\|component)-/,
		/^--spectrum-animation-/,

chore: update stylelint config settings #2406

chore: update stylelint config settings #2406

Conversation

castastrophe commented Jan 5, 2024 • edited by jawinn Loading

Description

How and where has this been tested?

To-do list

github-actions bot commented Jan 5, 2024 • edited Loading

File metrics

Summary

github-actions bot commented Jan 5, 2024 • edited Loading

jawinn left a comment • edited Loading

Choose a reason for hiding this comment

Plugins testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castastrophe commented Jan 10, 2024

jawinn commented Jan 10, 2024 • edited Loading

castastrophe commented Jan 10, 2024

castastrophe commented Jan 11, 2024

jawinn commented Jan 11, 2024 • edited Loading

castastrophe commented Jan 11, 2024 • edited Loading

castastrophe commented Jan 11, 2024

castastrophe commented Jan 5, 2024 •

edited by jawinn

Loading

github-actions bot commented Jan 5, 2024 •

edited

Loading

github-actions bot commented Jan 5, 2024 •

edited

Loading

jawinn left a comment •

edited

Loading

jawinn commented Jan 10, 2024 •

edited

Loading

jawinn commented Jan 11, 2024 •

edited

Loading

castastrophe commented Jan 11, 2024 •

edited

Loading