Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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/fluffy-peas-dress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris-migrator': minor
---

Adds new styles-replace-inline-comments general migration
Copy link
Member

@sam-b-rose sam-b-rose Sep 28, 2023

Choose a reason for hiding this comment

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

Suggested change
Adds new styles-replace-inline-comments general migration
Added `styles-replace-inline-comments` generic migration

1 change: 1 addition & 0 deletions polaris-migrator/codeshift.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {
'styles-insert-stylelint-disable',
),
'styles-replace-custom-property': resolve('styles-replace-custom-property'),
'styles-replace-inline-comments': resolve('styles-replace-inline-comments'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'styles-replace-inline-comments': resolve('styles-replace-inline-comments'),

Correct me if I'm wrong @sam-b-rose but we're not using presets right now, right? Tbh, I didn't even realize this file still existed after the consolidation of polaris-codemods and polaris-migrator.

Copy link
Member

Choose a reason for hiding this comment

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

I left this in as a just-in-case we wanted to try to use something like @codeshift/cli; however, I think we should delete this file. I think this change safe to be included in this PR.

'v11-react-update-page-breadcrumbs': resolve(
'v11-react-update-page-breadcrumbs',
),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// single-inline-comment
$single-inline-comment: 1rem;

// multiline
// comment
$multiline-comment: 1rem;

.comment {
$top-nested-comment: 1rem;
// nested-sandwidched-comment
$bottom-nested-comment: 1rem;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we confirm block comments /* */ don't show up in the build (Polaris and Web)? That was the primary reason we used inline comments // for Sass migrations.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, all comments are stripped in the minify step of our build process.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/* single-inline-comment */
$single-inline-comment: 1rem;

/* multiline */
/* comment */
$multiline-comment: 1rem;

.comment {
$top-nested-comment: 1rem;
/* nested-sandwidched-comment */
$bottom-nested-comment: 1rem;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import {check} from '../../../utilities/check';

const transform = 'styles-replace-inline-comments';
const fixtures = ['styles-replace-inline-comments'];

for (const fixture of fixtures) {
check(__dirname, {
fixture,
transform,
extension: 'scss',
options: {
customSyntax: 'postcss-scss',
reportDescriptionlessDisables: true,
rules: {},
},
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import type {API, FileInfo, Options} from 'jscodeshift';
import type {Plugin} from 'postcss';
import postcss from 'postcss';
import stylelint from 'stylelint';

const plugin = (): Plugin => {
return {
postcssPlugin: 'styles-replace-inline-comments',
Comment(comment) {
comment.raws.inline = false;
comment.raws.right = ' ';
comment.raws.left = ' ';
},
Comment on lines +9 to +13
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Comment(comment) {
comment.raws.inline = false;
comment.raws.right = ' ';
comment.raws.left = ' ';
},
Comment(comment) {
comment.raws.before = '\n';
comment.raws.inline = false;
comment.raws.right = ' ';
comment.raws.left = ' ';
},

GPT suggested using comment.raws.before 🤔 . Maybe conditionally depending on the current value of comment.raws.before, let's say if it's already \n.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so looked at this briefly yesterday and I suspect it's a bit more involved than this, because sometimes we want new break lines and some times we don't, and it depends on the prev() node.

I need to take a closer look to fine tune this, so I'll circle back to this in a few days 🤞

};
};

export default async function transformer(
file: FileInfo,
_: API,
options: Options,
) {
return postcss([
stylelint({
config: {
extends: [options.config ?? '@shopify/stylelint-polaris'],
Comment on lines +22 to +25
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand why we're using stylelint and @shopify/stylelint-polaris in this migration? We don't appear to be operating on the stylelint results in the migration plugin. I'm curious if we can follow the boilerplate Sass migration pattern (for example)

},
}) as Plugin,
plugin(),
])
.process(file.source, {
from: file.path,
syntax: require('postcss-scss'),
})
.then((result) => {
return result.css;
});
}