Skip to content

Commit

Permalink
[Polaris Lighthouse] Add migration to insert disable comments for @sh…
Browse files Browse the repository at this point in the history
…opify/stylelint-polaris (#7726)

**NOTE: even though this PR is independent of the [stylelint-polaris
v5](https://github.com/Shopify/polaris/pull/7691/files) release, I
recommend we hold off on shipping this until v5 to avoid confusion with
the migration, because running this script with v4 and v5 yield
different results**

### WHY are these changes introduced?

Fixes #7504

When we turn the stylelint-polaris linter on in codebases, we need a way
to ignore current warnings and errors since most if not all of these
failures would require some sort of refactor (there isn't a one-to-one
mapping of fixes).

This will ensure that refactors do not block us from turning on the
linter in order to "stop the bleeding" and prevent further regressions.

### WHAT is this pull request doing?

Creates a new styles migration called `styles-insert-stylelint-disable`
that goes through the given sass files and inserts a disable comment
like:

```diff
+ // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
padding: 1rem;
```

There is also logic to deal with edge cases like if there is already an
existing `stylelint-dsable-next-line` comment, it will merge the
descriptions. See more about the edge cases in the tests, or in the
`polaris-react` draft pr below (I left comments with some examples)

### How to 🎩
* Test on `polaris-react` #7773
(3140 failures -> 4 failures)
* Test on `shopify/web` Shopify/web#78162 (30383
failures -> 9 failures)

1. if testing in polaris-react, turn off the `overrides` in
`.stylelintrc.js`
2. run `npx stylelint 'polaris-react/**/*.scss'` and note all the
failures
3. run `yarn build && node ./bin/migrator-cli.js
styles-insert-stylelint-disable 'polaris-react/**/*.scss'`
4. run `npx stylelint 'polaris-react/**/*.scss'` and note the reduced
failures

If using a snapshot, run the migrator with `npx
@shopify/polaris-migrator styles-insert-stylelint-disable <path>`. The
migrator just uses a simple `@shopify/polaris-stylelint` config, so if
double checking with `npx stylelint` make sure you are running the same
version with the same config
  • Loading branch information
sophschneider committed Dec 1, 2022
1 parent 28529ba commit 22c4107
Show file tree
Hide file tree
Showing 9 changed files with 447 additions and 7 deletions.
5 changes: 5 additions & 0 deletions .changeset/funny-toys-buy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris-migrator': minor
---

Added migration to insert disable comments for @shopify/stylelint-polaris
14 changes: 14 additions & 0 deletions polaris-migrator/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ npx @shopify/polaris-migrator <migration> <path>

### v10

#### `styles-insert-stylelint-disable` (unstable)

Insert stylelint disable comments for [stylelint-polaris](../stylelint-polaris/) >= v5 so that
existing failures are not blocking a codebase from initializing the linter.

```diff
+ // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
padding: 1rem;
```

```sh
npx @shopify/polaris-migrator styles-insert-stylelint-disable <path>
```

#### `react-replace-text-component`

Replace legacy text components `DisplayText`, `Heading`, `Subheading`, `Caption`, `TextStyle`, and `VisuallyHidden` with the new single `Text` component.
Expand Down
4 changes: 3 additions & 1 deletion polaris-migrator/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,16 @@
},
"dependencies": {
"@shopify/polaris-tokens": "^6.3.0",
"@shopify/stylelint-polaris": "^4.4.0",
"chalk": "^4.1.0",
"globby": "11.0.1",
"is-git-clean": "^1.1.0",
"jscodeshift": "^0.13.1",
"meow": "^9.0.0",
"postcss": "^8.4.14",
"postcss-scss": "^4.0.4",
"postcss-value-parser": "^4.2.0"
"postcss-value-parser": "^4.2.0",
"stylelint": "^14.15.0"
},
"devDependencies": {
"@types/is-git-clean": "^1.1.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import os from 'os';

import type {FileInfo} from 'jscodeshift';
import postcss, {Node, Comment, Plugin, Root} from 'postcss';
import stylelint, {StylelintPostcssResult} from 'stylelint';

const crossPlatformNewlineRegExp = /\r\n?|\n/;
const polarisContextMsg = '-- generated by polaris-migrator DO NOT COPY';

const plugin = (): Plugin => {
return {
postcssPlugin: 'insert-stylelint-disable',
Once(root, {result}) {
result.messages.forEach(({node, line, endLine}) => {
// If a polaris ignore comment exists above the node already,
// do nothing
if (node.prev()?.text?.includes(polarisContextMsg)) {
return;
}

const isMultiline =
crossPlatformNewlineRegExp.test(node.value) ||
crossPlatformNewlineRegExp.test(node.text) ||
line < endLine;

const commentText = `${
isMultiline ? 'stylelint-disable' : 'stylelint-disable-next-line'
} ${polarisContextMsg}`;

const comment = createDisableComment(commentText, node.prev());

node.before(comment);

if (isMultiline) {
node.type === 'atrule' || node.type === 'rule'
? node.prepend(createCommentNode('stylelint-enable'))
: node.after(createCommentNode('stylelint-enable'));
}

deleteExtraNewlinesBeforeNode(node);
});

fillDescriptionlessDisables(result, root);
},
};
};

export default async function stylesInsertStylelintDisable(file: FileInfo) {
return postcss([
stylelint({
config: {
extends: ['@shopify/stylelint-polaris'],
// remove when stylelint-polaris v5 is released
reportDescriptionlessDisables: true,
},
}) as Plugin,
plugin(),
])
.process(file.source, {
from: file.path,
syntax: require('postcss-scss'),
})
.then((result) => {
return result.css;
});
}

/**
* Create a postcss comment node in the style:
* `// ${text}`
*/
function createCommentNode(text: string): Comment {
return new postcss.Comment({
text,
raws: {
left: ' ',
right: '',
inline: true,
},
});
}

/**
* Create a new disable comment with the given text.
* If the prevNode and the text are both disable-next-line
* comments, they are combined into a single comment with their
* description texts seperated by a comma.
*/
function createDisableComment(text: string, prevNode: Comment): Comment {
if (
prevNode?.type !== 'comment' ||
!prevNode.text.includes('stylelint-disable-next-line') ||
!text.includes('stylelint-disable-next-line')
) {
return createCommentNode(text);
}

const prevDescription = prevNode.text?.split('--')?.[1]?.trim();
const commentText = prevDescription?.length
? [text, prevDescription].join(', ')
: text;

prevNode.remove();

return createCommentNode(commentText);
}

/**
* Reduces the number of newline characters in a node's before
* raws to just one. This is helpful to ensure that there is
* only one newline between a disable-next-line comment and
* the warning node.
*/
function deleteExtraNewlinesBeforeNode(node: Node) {
node.raws.before = `${os.EOL}${node.raws.before.replace(
new RegExp(crossPlatformNewlineRegExp, 'g'),
'',
)}`;
}

/**
* If report-descriptionless-disables is enabled on stylelint,
* add a generated description to disables that don't have one.
*/
function fillDescriptionlessDisables(result: any, root: Root) {
const stylelintResult = result?.stylelint as StylelintPostcssResult;

if (stylelintResult?.config?.reportDescriptionlessDisables) {
root.walkComments((comment) => {
if (
comment.text.includes('stylelint-disable') &&
!comment.text.includes('--')
) {
comment.before(
createCommentNode([comment.text, polarisContextMsg].join(' ')),
);
comment.remove();
}
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
@import 'styles/common';

$no-extra-newlines-above-this: 200px;

.BaseCase {
padding: 1rem;
}

.NoInsert {
padding: var(--p-space-4);
}

.ExistingDisableComment {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
padding: 1rem;
}

.Nested {
.Keeps {
.Spacing {
padding: 1rem;
}
}
}

.CombineComment {
/* stylelint-disable-next-line my-original-rule -- original comment */
padding: 1rem;
/* stylelint-disable-next-line my-original-rule */
margin: 1rem;
/* stylelint-disable-next-line my-original-rule -- original comment */
margin-top: var(--p-space-1);
}

.MultiLineAtRule {
@media screen and (min-width: $largebreakpoint),
(min-width: $smallbreakpoint) and #{legacy-polaris-v8.$p-breakpoints-md-down} {
grid-column: 1;
}
}

.MultiLineDecl {
margin-top: calc(
(var(--pc-range-slider-thumb-size) - var(--pc-range-slider-track-height)) *
-0.5
);
}

.AnotherMultiLine {
$vertical-padding: calc(
(36px - var(--p-font-line-height-2) - var(--p-space-05)) / 2
);
}

.DescriptionlessDisable {
@media print {
// stylelint-disable-next-line declaration-no-important
padding: var(--p-space-4) !important;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
@import 'styles/common';

// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
$no-extra-newlines-above-this: 200px;

.BaseCase {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
padding: 1rem;
}

.NoInsert {
padding: var(--p-space-4);
}

.ExistingDisableComment {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
padding: 1rem;
}

.Nested {
.Keeps {
.Spacing {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
padding: 1rem;
}
}
}

.CombineComment {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY, original comment
padding: 1rem;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
margin: 1rem;
/* stylelint-disable-next-line my-original-rule -- original comment */
margin-top: var(--p-space-1);
}

.MultiLineAtRule {
// stylelint-disable -- generated by polaris-migrator DO NOT COPY
@media screen and (min-width: $largebreakpoint),
(min-width: $smallbreakpoint) and #{legacy-polaris-v8.$p-breakpoints-md-down} {
// stylelint-enable
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
grid-column: 1;
}
}

.MultiLineDecl {
// stylelint-disable -- generated by polaris-migrator DO NOT COPY
margin-top: calc(
(var(--pc-range-slider-thumb-size) - var(--pc-range-slider-track-height)) *
-0.5
);
// stylelint-enable
}

.AnotherMultiLine {
// stylelint-disable -- generated by polaris-migrator DO NOT COPY
$vertical-padding: calc(
(36px - var(--p-font-line-height-2) - var(--p-space-05)) / 2
);
// stylelint-enable
}

.DescriptionlessDisable {
@media print {
// stylelint-disable-next-line declaration-no-important -- generated by polaris-migrator DO NOT COPY
padding: var(--p-space-4) !important;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import {check} from '../../../utilities/testUtils';

const migration = 'styles-insert-stylelint-disable';
const fixtures = ['styles-insert-stylelint-disable'];

for (const fixture of fixtures) {
check(__dirname, {
fixture,
migration,
extension: 'scss',
});
}
30 changes: 25 additions & 5 deletions polaris-migrator/src/utilities/testUtils.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,28 @@
/* eslint-disable jest/no-export, jest/valid-title, @typescript-eslint/no-var-requires */
/* eslint-disable jest/no-export, jest/valid-title */
import fs from 'fs';
import path from 'path';

import jscodeshift, {FileInfo} from 'jscodeshift';
import prettier from 'prettier';

const applyTransform = require('jscodeshift/dist/testUtils').applyTransform;
export default async function applyTransform(
transform: any,
input: FileInfo,
options?: {[option: string]: any},
) {
// Handle ES6 modules using default export for the transform
const transformer = transform.default ? transform.default : transform;
const output = await transformer(
input,
{
jscodeshift: jscodeshift.withParser('tsx'),
stats: () => {},
},
options || {},
);

return (output || '').trim();
}

interface ParserExtensionMap {
[key: string]: prettier.BuiltInParserName;
Expand Down Expand Up @@ -38,9 +56,11 @@ export function check(
);
// Assumes transform is one level up from tests directory
const module = await import(path.join(dirName, '..', migration));
const output = applyTransform({...module, parser: 'tsx'}, options, {
source,
});
const output = await applyTransform(
{...module},
{source, path: inputPath},
options,
);

// Format output and expected with prettier for white spaces and line breaks consistency
expect(prettier.format(output, {parser})).toBe(
Expand Down

0 comments on commit 22c4107

Please sign in to comment.