[test] Introduce prettier into CI pipeline #12564

eps1lon · 2018-08-17T15:28:37Z

I realize that this might be bad time since CircleCI is broken but I encountered various other problems while working on this so I wanted to get it out.

This package currently uses prettier to format its code but does not verify that contributors check their format. With this change the CI pipeline will check changed files for their format.

While find enables piping to grep to check for changed files only it does not match its behavior to eslint and prettier. Prettier has its own --ignore-path argument and prettier:files could be removed in favor of prettier --ignore-path .eslintignore "**/*:{d.ts,js,tsx}".

However this caused performance degradation caused by prettier/prettier#4989. Timing both variants reveals little over 2x the runtime of the original command:

$ time find . -name "*.js" -o -name "*.d.ts" -o -name "*.tsx" | grep -v -f .eslintignore | xargs prettier --write
real    0m26.754s
user    0m34.860s
sys     0m2.356s

$ time prettier --ignore-path .eslintignore --write "**/*.{d.ts,js,tsx}"
real    1m2.349s
user    0m58.672s
sys     0m14.988s

Another issue is that the checked files changed:

--- prettier_files_find.sorted  2018-08-17 09:18:45.409774091 +0200
+++ prettier_files.sorted       2018-08-17 09:18:32.645896784 +0200
@@ -1,5 +1,8 @@
 ./.babelrc.js
 ./docs/.eslintrc.js
+./docs/scripts/buildApi.js
+./docs/scripts/buildIcons.js
+./docs/scripts/buildServiceWorker.js
 ./docs/src/config.js
 ./docs/src/modules/components/AppContent.js
 ./docs/src/modules/components/AppDrawer.js
@@ -329,21 +332,9 @@
 ./packages/eslint-plugin-material-ui/src/rules/docgen-ignore-before-comment.js
 ./packages/eslint-plugin-material-ui/src/rules/docgen-ignore-before-comment.test.js
 ./packages/material-ui-codemod/src/v0.15.0/import-path.js
-./packages/material-ui-codemod/src/v0.15.0/import-path.test/actual.js
-./packages/material-ui-codemod/src/v0.15.0/import-path.test/expected.js
-./packages/material-ui-codemod/src/v0.15.0/import-path.test.js
 ./packages/material-ui-codemod/src/v1.0.0/color-imports.js
-./packages/material-ui-codemod/src/v1.0.0/color-imports.test/actual.js
-./packages/material-ui-codemod/src/v1.0.0/color-imports.test/expected.js
-./packages/material-ui-codemod/src/v1.0.0/color-imports.test.js
 ./packages/material-ui-codemod/src/v1.0.0/import-path.js
-./packages/material-ui-codemod/src/v1.0.0/import-path.test/actual.js
-./packages/material-ui-codemod/src/v1.0.0/import-path.test/expected.js
-./packages/material-ui-codemod/src/v1.0.0/import-path.test.js
 ./packages/material-ui-codemod/src/v1.0.0/svg-icon-imports.js
-./packages/material-ui-codemod/src/v1.0.0/svg-icon-imports.test/actual.js
-./packages/material-ui-codemod/src/v1.0.0/svg-icon-imports.test/expected.js
-./packages/material-ui-codemod/src/v1.0.0/svg-icon-imports.test.js
 ./packages/material-ui-docs/scripts/copy-files.js
 ./packages/material-ui-docs/src/index.js
 ./packages/material-ui-docs/src/MarkdownElement/index.js
@@ -356,6 +347,8 @@
 ./packages/material-ui-docs/src/svgIcons/LightbulbFull.js
 ./packages/material-ui-docs/src/svgIcons/LightbulbOutline.js
 ./packages/material-ui-docs/src/svgIcons/Twitter.js
+./packages/material-ui-icons/builder.js
+./packages/material-ui-icons/builder.test.js
 ./packages/material-ui-icons/.eslintrc.js
 ./packages/material-ui-icons/renameFilters/default.js
 ./packages/material-ui-icons/renameFilters/material-design-icons.js
@@ -1262,6 +1255,7 @@
 ./pages/guides/api.js
 ./pages/guides/composition.js
 ./pages/guides/csp.js
+./pages/guides/flow.js
 ./pages/guides/interoperability.js
 ./pages/guides/migration-v0x.js
 ./pages/guides/minimizing-bundle-size.js

Since using --ignore-path considers the same files as eslint we should switch at some point to --ignore-path.

I have another iteration that uses --ignore-path at least in ci since the slower runtime is negligible since we only run on the changed files.

For an explanation as to why prettier only runs on changed files and not on all see the commit message of 7d42ca0.

oliviertassinari · 2018-08-17T17:33:36Z

does not verify that contributors check their format.

We do, we have an eslint plugin for it. Now, TypeScript handling is different. We don't run eslint on TypeScript, but I'm wondering if we shouldn't.

eps1lon · 2018-08-17T18:01:43Z

So how do I trigger this? `yarn test` did not throw which does run lint if I remember correctly. For typescript we should use tslint I think.

…

On Fri, Aug 17, 2018, 19:34 Olivier Tassinari ***@***.***> wrote: does not verify that contributors check their format. We do, we have an eslint plugin for it. Now, TypeScript handling is different. We don't run eslint on TypeScript, but I'm wondering if we shouldn't. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12564 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALuPzwyrgXOy_pDuFY4uKsgJFjdNbEMhks5uRv6XgaJpZM4WBwp2> .

oliviertassinari · 2018-08-17T20:52:39Z

CircleCI is broken

It's definitely broken: https://discuss.circleci.com/t/saving-cache-stopped-working-warning-skipping-this-step-disabled-in-configuration/24423.

how do I trigger this?

yarn test triggers it, but we use yarn lint directly in the CI, same behavior. I don't think that it's a coincidence the only files that changed are the TypeScript ones. I believe we would have way more diffs if prettier was actually broken for the JavaScript files in the CI.

oliviertassinari · 2018-08-17T20:55:25Z

.circleci/config.yml

+            # Files changed on this branch
+            CHANGED_FILES=$(git diff --name-only master...)
+            # Files that should have been formatted while working on this branch
+            FORMATTED_FILES=$(yarn --silent prettier:files | grep "$CHANGED_FILES")


Do we run prettier twice here? Would it be faster to run it once and to execute git diff --exit-code?
Also, I think that we scope the new check to the TypeScript files :).

find always did pipe *.d.ts and *.tsx to the prettier script. eslint only processes jsx? files though as far as I know.

I was never really a fan of eslint-plugin-prettier. Mostly this comes down to personal preference but also some actual minor issues: eslint has no parser for ts files which makes eslint --fix with eslint-plugin-prettier useless for ts files. eslint-plugin-prettier allows prettier configuration with .eslintrc which is ignored by ide plugins.

prettier:files is just a helper method at the moment which lists files that should be formatted. It is not actually running prettier --write.

prettier --write should never be used in a CI environment. That's the hole purpose of --list-different. Combine --write with git diff while reducing I/O operations.

It was intended that the new check enforces a consistent code style across ts and js file. Just like the existing prettier scripts already formats tsx? files.

Now I understood what you mean. Since we run prettier via eslint we would run it here again on all jsx? files. I removed eslint-plugin-prettier to removed this redundancy.

eslint --fix

I never used that. I have never been satisfied with the output.

I was never really a fan of eslint-plugin-prettier

Most people configure their editor to run eslint. The advantage of this plugin is that you know up front that you need to run prettier, it's just like another eslint rule. At least, it's my workflow. I don't automatically run prettier on save. This data is interesting: https://npm-stat.com/charts.html?package=eslint-plugin-prettier&package=prettier.

Well prettier supports more than jsx? while eslint only runs on jsx?. Typescript support might explain some difference.

I didn't think about editor integration though and while I also don't use formatOnSave I do frequently press the format hotkey.

A file can be formatted without knowledge about other files. For this reason we only check the format for files which were changed compared to master. A broken format that was merged into master will not be picked up by CI until the file was changed again. This can be a good thing since this will not trigger CI errors in unrelated builds and since we are only talking about code style this a manageable tradeoff especially because formatting can be very expensive.

oliviertassinari · 2018-08-18T11:58:56Z

@eps1lon Ok, let's try this workflow without eslint-plugin-prettier. I will add it back if I feel the need to. Thank you for this new CI check :)

oliviertassinari · 2018-08-18T12:12:00Z

It saves ~20s in the lint duration.

oliviertassinari reviewed Aug 17, 2018

View reviewed changes

oliviertassinari changed the title ~~Introduce prettier into CI pipeline~~ [test] Introduce prettier into CI pipeline Aug 17, 2018

oliviertassinari added the test label Aug 17, 2018

eps1lon force-pushed the ci-check-format branch from fb4ac72 to 55f217f Compare August 18, 2018 11:04

eps1lon added 3 commits August 18, 2018 13:26

[core] run prettier

39f749f

[test] use prettier:check instead of implicit run via eslint

69e63ed

eps1lon force-pushed the ci-check-format branch from 55f217f to 69e63ed Compare August 18, 2018 11:27

oliviertassinari merged commit fb96d23 into mui:master Aug 18, 2018

eps1lon mentioned this pull request Aug 18, 2018

[test] Fix prettier ci task with multiple changed files #12579

Merged

eps1lon deleted the ci-check-format branch August 23, 2018 06:03

eps1lon mentioned this pull request Nov 11, 2018

[core] Simplify Prettier Scripts #13571

Closed

1 task

eps1lon mentioned this pull request Jan 12, 2019

[core] Remove unnecessary plugins in .eslintrc #14161

Merged

1 task

snyk-bot mentioned this pull request Sep 30, 2019

[Snyk] Upgrade @material-ui/core from 1.0.0 to 1.5.1 Matthelonianxl/nifty-wallet#7

Open

This was referenced Mar 26, 2020

[Snyk] Upgrade @material-ui/core from 1.0.0 to 1.5.1 AndreiShybin/metamask-extension#9

Merged

[Snyk] Upgrade @material-ui/core from 1.0.0 to 1.5.1 baophucct/metamask-extension#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Introduce prettier into CI pipeline #12564

[test] Introduce prettier into CI pipeline #12564

eps1lon commented Aug 17, 2018

oliviertassinari commented Aug 17, 2018

eps1lon commented Aug 17, 2018 via email

oliviertassinari commented Aug 17, 2018

oliviertassinari Aug 17, 2018 •

edited

Loading

eps1lon Aug 17, 2018

eps1lon Aug 18, 2018

oliviertassinari Aug 18, 2018

eps1lon Aug 18, 2018

oliviertassinari commented Aug 18, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018

[test] Introduce prettier into CI pipeline #12564

[test] Introduce prettier into CI pipeline #12564

Conversation

eps1lon commented Aug 17, 2018

oliviertassinari commented Aug 17, 2018

eps1lon commented Aug 17, 2018 via email

oliviertassinari commented Aug 17, 2018

oliviertassinari Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

eps1lon Aug 17, 2018

Choose a reason for hiding this comment

eps1lon Aug 18, 2018

Choose a reason for hiding this comment

oliviertassinari Aug 18, 2018

Choose a reason for hiding this comment

eps1lon Aug 18, 2018

Choose a reason for hiding this comment

oliviertassinari commented Aug 18, 2018 • edited Loading

oliviertassinari commented Aug 18, 2018

oliviertassinari Aug 17, 2018 •

edited

Loading

oliviertassinari commented Aug 18, 2018 •

edited

Loading