How should we treat Stylelint Polaris warnings in Shopify/web?

[sophschneider]

Right now, Shopify/web's CI fails stylelint on both warnings and errors. This doesn't allow us to tier warnings and failures from Stylelint Polaris. Having warnings could allow us to nudge developers to fix coverage problems without blocking them which would be useful for rules like layout.

Possible solutions

Feel free to comment more solutions if you have one!

Lets say one day when we enable the Layout stylelint rules, someone:

• adds display: flex (which is a warning)
• does NOT add a context message (e.g. // stylelint-disable-next-line -- we need flex because...)

1. Make them add a disable context message

This is equivalent to what we have today failing on both warnings and errors. Once a developer adds a disable context message, both errors and warnings will pass

• Pros:
  • We have context on why a rule is being disabled
  • We don't have to make a change in web
  • Stylelint CI logs are kept clean of noise
  • We know that all stylelint warnings are either disabled or fixed.
• Cons:
  • The treatment of warnings and errors are the same
  • Enforces action (either disabling the rule or fixing the warning) which could be annoying for warning rules

2. Create two parallel CI steps for Stylelint in Shopify/Web

In VSCode we would use the joint config so that all errors and warnings are shown in there. On CI, we could run one step with the basic shopify/web stylelint config and one step with the stylelint polaris config. We could even modify our Polaris step to not report warnings at all in the CI so it doesn't get noisy and the warning experience would only be in VSCode.

• Pros:
  • Doesn't make Shopify/web's main stylelinter CI logs noisy.
  • Can modify our stylelint polaris CI step to use any config we want (including not reporting warnings so the console doesn't get noisy at all)
  • Puts all Polaris errors and warnings in one place
  • We could explore nudging developers in other ways to fix/disable their warnings via cautiontape bot or a slack bot?
• Cons:
  • We aren't gathering context about warnings
  • Developers might miss warnings
  • Warning failures can still accumulate in our codebase

3. Let them ship the PR (don't fail stylelint warnings in web)

• Pros:
  • Stylelint Polaris won't be strict for warnings and will allow the nudge to show up in VSCode
  • We could explore nudging developers in other ways to fix/disable their warnings via cautiontape bot or a slack bot?
• Cons:
  • Shopify/web currently fails stylelint warnings so that warnings aren't shipped to the codebase. Changing this would be pretty invasive on Polaris' part without a good reason or a good way of dealing with warning noise
  • Developers wouldn't look at the warnings in CI if it passed all failures
  • The Stylelint CI logs could pile up warnings so when something does fail on a PR, it will be hard to find what actually failed and there could be a bunch of warnings on things that are not in that developer's PR
  • Without enforcing disable comments, we won't have any context on why developers are disabling stylelint polaris warning rules

[tmlayton]

I’m partial to no. 2 or 3. I don’t think warnings should be blocking where disabled comments are required. It would equate to a lot of time for developers and I’m not confident the disabled comments would give us a strong signal vs noise.

If we have clear guidance around resolving a specific warning, arguably it can be turned into an error.

In addition to seeing the warning in VSCode I like exploring surfacing warnings async and non-blocking with caution tape bot or something similar where we could use that to potentially solicit feedback, educate about the warning, or educate about adding a reason via comment, etc.

My gut feeling is a majority of warnings can be ignored entirely and the code is needed, but there is a chunk or warnings that are due to not using the components/tokens properly, shortcomings of components/tokens, etc.

[sam-b-rose]

+1 with Tim's points here. I am also partial to 2 or 3.

Option 2 seems like a less invasive first step that would be non-blocking but provide a distinction between warnings and errors while reserving the need overly use the disable comment escape-hatch.

This option might have the most work required to setup, but I think it would be the ideal next step.

[sophschneider]

Thanks @samrose3 and @tmlayton for your input! I'm going to go with #2 as I agree with both of your points and this will be less friction with the web platform's current stylelint step.