Prototype a stylelint shim #7464
Conversation
| // padding: var(--p-space-4) 10rem; | ||
| padding: 16px 10rem; | ||
| // polaris-migrator: Unable to migrate the following expression. Please upgrade manually. | ||
| // error: Non-tokenizable value '10px' | ||
| // padding: 10px var(--p-space-4); |
There was a problem hiding this comment.
Still outputs the partial fix where possible, but enhances that with the specific reason it couldn't be fully fixed.
Huge DX win!
| @@ -23,55 +25,65 @@ | |||
| padding: var(--p-space-4, 16px); | |||
| // Comment | |||
| // polaris-migrator: Unable to migrate the following expression. Please upgrade manually. | |||
| // padding: 10px; | |||
| // error: Non-tokenizable value '10px' | |||
There was a problem hiding this comment.
These error messages will be the exact same as exposed by stylelint once we migrate there. 🎉
| ); | ||
| } else { | ||
| decl.value = parsedValue.toString(); | ||
| } |
There was a problem hiding this comment.
Begone repeated boilerplate code! 🪄
|
I think my question is: In what cases would we want migration checks and fixes to exist in perpetuity? Stylelint rules hang around forever - they spot mistakes that could be made and fix them. However the modifications in migrations tend to be one and done. You apply a migration like "replace usage of That said, if the plan is "We stop using codemod for migrations, instead these fixes are exposed as a stylelint rules that contains autofixes where appropriate" then that sounds legit. |
|
@BPScott Yes, this is a fantastic question! There's really two distinct times a developer might want their code automatically fixed for them:
For #1, we have For #2, we have These two use-cases are distinct, but it turns out they can use the exact same tech stack: The "one and done" migrations would be separate rules (bundled into All of that is future-looking. This PR is mearly the first step in that direction (one I hope I'm aligned with the rest of the team on 😅) |
jesstelford
force-pushed
the
prototype-stylelint-shim
branch
from
07dca91 to
bfa85dc
Compare
|
|
||
| // Expose a stylelint-like API for creating sass migrators so we can easily | ||
| // migrate to that tool in the future. | ||
| function convertStylelintRuleToPostcssProcessor(ruleFn: StylelintRule) { |
There was a problem hiding this comment.
This is the shim function.
Approx 90% of this function would be deleted by migrating to stylelint, meaning this function is just a shim to get postcss's API to look like stylelint's (for now).
| partialFixStrategy: 'report', | ||
| __injectReportsAsComments: true, | ||
| }, | ||
| {fix: true}, |
There was a problem hiding this comment.
The fix option is passed by stylelint when the cli switch --fix is passed. Since this is within our migrator cli, we hard-code it to true for this shim.
| { | ||
| ...options, | ||
| partialFixStrategy: 'report', | ||
| __injectReportsAsComments: true, |
There was a problem hiding this comment.
This is an example of receiving options via stylelint rule config. In this case, we're instructing the migrations to insert comments (because that's the only way to get output from postcss), but it could also be set to emit stylelint "reports" which are then consumable by other tools such as LSPs, VSCode, etc.
| (existingReport) => | ||
| existingReport.severity === newReport.severity && | ||
| existingReport.message === newReport.message, | ||
| ) === -1 |
There was a problem hiding this comment.
Deduping reports to avoid noise. Particularly useful for example padding: 10px 10px 10px 10px - we only want 1 warning, not 4 identical ones.
| }; | ||
| } | ||
|
|
||
| export function createStylelintRule(ruleName: string, ruleFn: PolarisMigrator) { |
There was a problem hiding this comment.
This factory function matches the stylelint API as closely as possible. There are only a couple of lines in this function which would be removed once we move to stylelint.
| | ((node: T, parsedValue: ParsedValue) => false | void); | ||
| mutableKeys?: (keyof T)[]; | ||
| serialiseSuggestion: (node: T) => string; | ||
| }): (node: T) => false | void { |
| mutableKeys?: (keyof T)[]; | ||
| serialiseSuggestion: (node: T) => string; | ||
| }): (node: T) => false | void; | ||
| function createWalker<T extends PostCSSNode>(args: { |
There was a problem hiding this comment.
This factory function wraps up common logic across all of our migrators:
- Tracking if changes have occurred to a postcss Node (previously done manually in each migration via a
targetsarray) - Adding comments if there's a partial migration
- Add comments when there's an error/warning detected, but it's unmigratable
| walkAtRules, | ||
| walkComments, | ||
| walkDecls, | ||
| walkRules, |
There was a problem hiding this comment.
These util methods cover most of the postcss Root API that our migrations will/might use.
|
This is an excellent exploration aligning our migration structure with how Stylelint structures rules. This iteration does bring the abstraction closer to Stylelint's API, but it also introduces a lot of complexity and changes to our current migrations–even if only temporary. Until we are ready and sure we'd like to migrate the migrations to our Stylelint plugin, I'd like to let this work simmer if that is alright 🫕. This will certainly be valuable in helping form our strategy for integrating |
jesstelford
force-pushed
the
prototype-stylelint-shim
branch
from
268804c to
72cb3c3
Compare
|
Fair call: This PR has a lot going on. I'll split out a couple things I think will be useful for us to have in separate PRs 👍 |
tl;dr: I wrote a shim which should simplify future work to improve the coverage tool. It'll help with code maintenance today, with the goal of making our coverage more accurate and the
stylelint-polaristool more useful in the future. See the migration template for a minimal usage example.In conversation with @aaronccasanova yesterday, he pointed out that except for some extreme edge cases, all our migrations follow an identical pattern of:
display:declarations, for example)In seeing that pattern, he's been able to simplify logic and converge on a nice code structure as exemplified by the
replace-spacing-lengthsmigration.I began shoring up the
motionmigration to follow this structure and started to see that @aaronccasanova's structure could be extended further to remove duplication and make every migration even more consistent + easy to maintain.At the same time, I've been thinking on how our coverage tooling (powered by
stylelint) and migration tooling (powered bypostcss) relate, and specifically how they're starting to diverge, and wondering what we can do about it. I discovered thatstylelintsupports running "migrations" via its built-in--fixoption. I immediately thought of a future where we usedstylelintto execute ourpostcssmigrations.It turns out
stylelintis actually powered bypostcss! And the the way Stylelint rules function is:display:declarations, for example)--fixis passedWhich is almost identical to @aaronccasanova's observation and subsequent code structure 🤯
So, much like Alice, I tumbled down the Rabbit Hole :rabbit-hole: and realised that with just a little shim function, we could write our
postcssmigrations today such that they're compatible with being run withinstylelintin the future, while ensuring the migrations are consistent and easy to maintain ✅I'd love to hear thoughts! Is this useful? Is it confusing? Am I aiming in the right direction?