Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

More robust stylelint shim to support reporting #7532

Merged
merged 1 commit into from
Nov 2, 2022
Merged

More robust stylelint shim to support reporting #7532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/seven-zoos-sin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris-migrator': minor
---

Expose the .report() method to SASS migrations for easier aggregation of discovered issues during a migration run.
62 changes: 37 additions & 25 deletions polaris-migrator/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,38 +278,50 @@ migrations

#### The SASS migration function

Each migrator has a default export adhering to the [PostCSS Plugin API](https://github.com/postcss/postcss/blob/main/docs/writing-a-plugin.md) with one main difference: events are only executed once.
Each migrator has a default export adhering to the [Stylelint Rule API](https://github.com/postcss/postcss/blob/main/docs/writing-a-plugin.md). A PostCSS AST is passed as the `root` and can be mutated inline, or emit warning/error reports.

Continuing the example, here is what the migration may look like if our goal is to replace the Sass function `hello()` with `world()`.

```ts
// polaris-migrator/src/migrations/replace-sass-function/replace-sass-function.ts

import type {FileInfo} from 'jscodeshift';
import postcss, {Plugin} from 'postcss';
import valueParser from 'postcss-value-parser';

const plugin = (): Plugin => ({
postcssPlugin: 'replace-sass-function',
Declaration(decl) {
// const prop = decl.prop;
const parsedValue = valueParser(decl.value);

parsedValue.walk((node) => {
if (!(node.type === 'function' && node.value === 'hello')) return;

node.value = 'world';
import {
isSassFunction,
StopWalkingFunctionNodes,
createSassMigrator,
} from '../../utilities/sass';
import type {PolarisMigrator} from '../../utilities/sass';

const replaceHelloWorld: PolarisMigrator = (_, {methods}, context) => {
return (root) => {
root.walkDecls((decl) => {
const parsedValue = valueParser(decl.value);
parsedValue.walk((node) => {
if (isSassFunction('hello', node)) {
if (context.fix) {
node.value = 'world';
} else {
methods.report({
node: decl,
severity: 'error',
message:
'Method hello() is no longer supported. Please migrate to world().',
});
}

return StopWalkingFunctionNodes;
}
});

if (context.fix) {
decl.value = parsedValue.toString();
}

methods.flushReports();
});
};
};

decl.value = parsedValue.toString();
},
});

export default function replaceSassFunction(fileInfo: FileInfo) {
return postcss(plugin()).process(fileInfo.source, {
syntax: require('postcss-scss'),
}).css;
}
export default createSassMigrator('replace-hello-world', replaceHelloWorld);
```

A more complete example can be seen in [`replace-spacing-lengths.ts`](https://github.com/Shopify/polaris/blob/main/polaris-migrator/src/migrations/replace-spacing-lengths/replace-spacing-lengths.ts).
Expand Down
117 changes: 72 additions & 45 deletions polaris-migrator/src/migrations/replace-spacing-lengths/replace-spacing-lengths.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import valueParser from 'postcss-value-parser';

import {POLARIS_MIGRATOR_COMMENT} from '../../constants';
import {
createInlineComment,
getFunctionArgs,
isNumericOperator,
isSassFunction,
Expand All @@ -16,44 +14,42 @@ import {isKeyOf} from '../../utilities/type-guards';

export default createSassMigrator(
'replace-sass-space',
(_, options, context) => {
(_, {methods, options}, context) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a methods object or can we import things from an external utility like Stylelint (stylelint.utils.report)?

const namespacedRem = namespace('rem', options);

return (root) => {
root.walkDecls((decl) => {
if (!spaceProps.has(decl.prop)) return;

/**
* A collection of transformable values to migrate (e.g. decl lengths, functions, etc.)
*
* Note: This is evaluated at the end of each visitor execution to determine whether
* or not to replace the declaration or insert a comment.
*/
const targets: {replaced: boolean}[] = [];
let hasNumericOperator = false;
const parsedValue = valueParser(decl.value);

handleSpaceProps();

if (targets.some(({replaced}) => !replaced || hasNumericOperator)) {
decl.before(
createInlineComment(POLARIS_MIGRATOR_COMMENT, {prose: true}),
);
decl.before(
createInlineComment(`${decl.prop}: ${parsedValue.toString()};`),
);
} else if (context.fix) {
decl.value = parsedValue.toString();
const newValue = parsedValue.toString();

if (context.fix && newValue !== decl.value) {
if (methods.getReportsForNode(decl)) {
// The "partial fix" case: When there's a new value AND a report.
methods.report({
node: decl,
severity: 'suggestion',
message: `${decl.prop}: ${parsedValue.toString()}`,
});
} else {
decl.value = parsedValue.toString();
}
}

//
// Handlers
//
methods.flushReports();

function handleSpaceProps() {
parsedValue.walk((node) => {
if (isNumericOperator(node)) {
hasNumericOperator = true;
methods.report({
node: decl,
severity: 'warning',
message: 'Numeric operator detected.',
});
return;
}

Expand All @@ -64,41 +60,72 @@ export default createSassMigrator(

if (!isTransformableLength(dimension)) return;

targets.push({replaced: false});

const valueInPx = toTransformablePx(node.value);

if (!isKeyOf(spaceMap, valueInPx)) return;
if (!isKeyOf(spaceMap, valueInPx)) {
methods.report({
node: decl,
severity: 'error',
message: `Non-tokenizable value '${node.value}'`,
});
return;
}

targets[targets.length - 1]!.replaced = true;
if (context.fix) {
node.value = `var(${spaceMap[valueInPx]})`;
return;
}

node.value = `var(${spaceMap[valueInPx]})`;
methods.report({
node: decl,
severity: 'error',
message: `Prefer var(${spaceMap[valueInPx]}) Polaris token.`,
});
return;
}

if (node.type === 'function') {
if (isSassFunction(namespacedRem, node)) {
targets.push({replaced: false});

const args = getFunctionArgs(node);

if (args.length !== 1) return;
if (args.length !== 1) {
methods.report({
node: decl,
severity: 'error',
message: `Expected 1 argument, got ${args.length}`,
});
return;
}

const valueInPx = toTransformablePx(args[0]);

if (!isKeyOf(spaceMap, valueInPx)) return;

targets[targets.length - 1]!.replaced = true;

node.value = 'var';
node.nodes = [
{
type: 'word',
value: spaceMap[valueInPx],
sourceIndex: node.nodes[0]?.sourceIndex ?? 0,
sourceEndIndex: spaceMap[valueInPx].length,
},
];
if (!isKeyOf(spaceMap, valueInPx)) {
methods.report({
node: decl,
severity: 'error',
message: `Non-tokenizable value '${args[0].trim()}'`,
});
return;
}

if (context.fix) {
node.value = 'var';
node.nodes = [
{
type: 'word',
value: spaceMap[valueInPx],
sourceIndex: node.nodes[0]?.sourceIndex ?? 0,
sourceEndIndex: spaceMap[valueInPx].length,
},
];
return;
}

methods.report({
node: decl,
severity: 'error',
message: `Prefer var(${spaceMap[valueInPx]}) Polaris token.`,
});
}

return StopWalkingFunctionNodes;
Expand Down
35 changes: 26 additions & 9 deletions ...migrator/src/migrations/replace-spacing-lengths/tests/replace-spacing-lengths.output.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
padding: 0;
padding: 1;
padding: 2;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// warning: Numeric operator detected.
padding: #{16px + 16px};
padding: layout-width(nav);
padding: 10em;
Expand All @@ -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'
padding: 10px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: 10rem;
// error: Non-tokenizable value '10rem'
padding: 10rem;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: 10px 10px;
// error: Non-tokenizable value '10px'
padding: 10px 10px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '10px'
// padding: var(--p-space-4) 10px;
padding: 16px 10px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: 10rem 10rem;
// error: Non-tokenizable value '10rem'
padding: 10rem 10rem;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '10rem'
// padding: var(--p-space-4) 10rem;
padding: 1rem 10rem;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: 10px 10rem;
// error: Non-tokenizable value '10px'
// error: Non-tokenizable value '10rem'
padding: 10px 10rem;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '10rem'
// 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);
padding: 10px 1rem;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '-16px'
// padding: var(--p-space-4) -16px;
padding: 16px -16px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// warning: Numeric operator detected.
// padding: var(--p-space-4) + var(--p-space-4);
padding: 16px + 16px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// warning: Numeric operator detected.
// padding: var(--p-space-4) + var(--p-space-4) var(--p-space-4);
padding: 16px + 16px 16px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// warning: Numeric operator detected.
// padding: $var + var(--p-space-4);
padding: $var + 16px;
padding: calc(16px + 16px);
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// warning: Numeric operator detected.
// padding: var(--p-space-4) + #{16px + 16px};
padding: 16px + #{16px + 16px};
// Skip negative lengths. Need to discuss replacement strategy e.g.
// calc(-1 * var(--p-space-*)) vs var(--p-space--*)
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: -16px;
// error: Non-tokenizable value '-16px'
padding: -16px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: -10px;
// error: Non-tokenizable value '-10px'
padding: -10px;

// REM FUNCTION
Expand All @@ -86,21 +98,26 @@
padding: calc(10rem + var(--p-choice-size, #{rem(10px)}));
// Comment
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: rem(10px);
// error: Non-tokenizable value '10px'
padding: rem(10px);
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// padding: rem(10px) rem(10px);
// error: Non-tokenizable value '10px'
padding: rem(10px) rem(10px);
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '10px'
// padding: var(--p-space-4) rem(10px);
padding: rem(16px) rem(10px);
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '-16px'
// padding: var(--p-space-4) -16px;
padding: rem(16px) -16px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// warning: Numeric operator detected.
// padding: var(--p-space-4) + var(--p-space-4);
padding: rem(16px) + 16px;
// polaris-migrator: Unable to migrate the following expression. Please upgrade manually.
// error: Non-tokenizable value '$var \* 16px'
// warning: Numeric operator detected.
// padding: rem($var * var(--p-space-4));
padding: rem($var * 16px);
}
Loading