Skip to content

chore: fix Stylelint globs for editor support#6476

Merged
Josh-Cena merged 7 commits into
facebook:mainfrom
nschonni:stylelint-globs
Jan 28, 2022
Merged

chore: fix Stylelint globs for editor support#6476
Josh-Cena merged 7 commits into
facebook:mainfrom
nschonni:stylelint-globs

Conversation

@nschonni
Copy link
Copy Markdown
Contributor

Motivation

Stylelint will try to run on other files in IDEs. This expands out the globing from the CLI to include all files, by updating the ignores to just the files that are impacted. Added the parsers for HTML and MD files, but left the others like TS(X) since I don't think there is CSS in JS used here

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

Comment thread package.json Outdated
"stylelint --allow-empty-input --fix"
],
"*": [
"stylelint --allow-empty-input --fix",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm still skeptical about using stylelint to fix non-CSS code though, don't like any of the diff in the MD files.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what do you dislike?

I don't have a strong opinion on this but we already use similar rgb strings in our theme CSS files

.copyButton {
  background: rgb(0 0 0 / 30%);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fair enough, I just feel like what we present to the user doesn't necessarily have to follow our internal code style, but rather what's most popular (and I guess the legacy function style is still more widely recognized)... I'm fine with both.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree on that, and also found this syntax less popular

maybe we should just change the rules, so that the most popular syntax also applied to our theme (hence swizzling too)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But I do like the modern syntax better :P Don't really want to tweak our code style, just want to enforce control over our text content. ESLint doesn't enforce anything on Markdown code blocks.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm fine if we disable this on all md files or just this specific rule, that doesn't seem to have a huge impact anyway

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, I don't really want to use it to lint anything other than CSS. We have Prettier to format the inline code anyways.

@nschonni
Copy link
Copy Markdown
Contributor Author

The build error is interesting, since it looks like there is a transitive dependency that needs to be pinned. I tried updating the remark plug-ins in the migrate package, but there is still a build issue

@Josh-Cena
Copy link
Copy Markdown
Collaborator

This seems to be because an alternative version of unified (v10) is installed at the root and unified@8 is now installed in the migrate subfolder, which then causes the wrong type to be resolved...

@Josh-Cena
Copy link
Copy Markdown
Collaborator

Josh-Cena commented Jan 27, 2022

Oops, it's because of the wrong unified version being hoisted 😅

Problem is this: remark-parse imports unified for its types; migrate then imports both for their types. Before, remark-parse and unified with the versions migrate expects are hoisted to the root; but now, a newer version of unified is hoisted, because the facebook template is installed first which depends on a newer version of unified (because of the stylelint bump). However, because remark-parse only imports types from unified, the latter isn't in its dependencies. That causes the wrong types of unified to be imported and hence a mistyped remark-parse plugin.

@Josh-Cena Josh-Cena changed the title chore: expand Stylelint globs chore: fix Stylelint globs for editor support Jan 27, 2022
@Josh-Cena
Copy link
Copy Markdown
Collaborator

Reverted the stylelint bump in the Facebook template. We'll figure that out later...

@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Jan 28, 2022
@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 28, 2022

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 5c7518a

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61f3689494809c0007953e43

😎 Browse the preview: https://deploy-preview-6476--docusaurus-2.netlify.app

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 28, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 94
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-6476--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena merged commit bcb1533 into facebook:main Jan 28, 2022
@nschonni nschonni deleted the stylelint-globs branch January 28, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants