Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Simplify Prettier Scripts #13571

Closed
wants to merge 1 commit into from

Conversation

joshwooding
Copy link
Member

I was having some issues with the prettier scripts printing errors to my console, so I simplified them and fixed the errors.

@mbrookes
Copy link
Member

That looks much tidier. 👍

mbrookes
mbrookes previously approved these changes Nov 11, 2018
@mbrookes
Copy link
Member

I don't know why, but when I run prettier, it changes some files. It isn't to do with this PR though - it does it anyway.

@eps1lon
Copy link
Member

eps1lon commented Nov 11, 2018

Could you add some time measurements for the old and new command? If I recall correctly this should be much slower because of prettier/prettier#4989.

@mbrookes
Copy link
Member

@eps1lon I did notice it was slow - thought it was just my machine, but a benchmark would be a good idea.

@mbrookes mbrookes dismissed their stale review November 11, 2018 15:41

Performance issue.

@joshwooding
Copy link
Member Author

Sadly my benchmark won't be a true representation as prettier is formatting the ignored files. If I run the prettier:files command in my console it works as intended but as soon as I run it with npm or yarn it produces a different output 😕

@eps1lon
Copy link
Member

eps1lon commented Nov 11, 2018

What do you mean by different output?

@joshwooding
Copy link
Member Author

find . -name \"*.js\" -o -name \"*.d.ts\" -o -name \"*.tsx\" | grep -v -f .eslintignore

Prints material-ui-icons/src , node_modules and more. The yarn command behaves the same

find . -name \"*.js\" -o -name \"*.d.ts\" -o -name \"*.tsx\" | grep -v "$(cat .eslintignore)"

Doesn't print node_modules but prints material-ui-icons/src and .next files etc.

yarn prettier with find . -name \"*.js\" -o -name \"*.d.ts\" -o -name \"*.tsx\" | grep -v "$(cat .eslintignore)"

Will print node_modules again and all the other files.

@joshwooding
Copy link
Member Author

For performance checks on my machine:

Current Solution:
real 12m1.390s
user 0m0.061s
sys 0m0.462s

Shortened Solution:
real 2m34.281s
user 0m0.060s
sys 0m0.275s

Using CLI arguments:
real 0m45.531s
user 0m0.045s
sys 0m0.245s

If I can get the find + grep combo working, I think that will be the best way forward as for the last benchmark I had to glob-ify the .eslintignore entries

@joshwooding
Copy link
Member Author

Although this isn't simpler cat .eslintignore | tr -s '\\r\\n' ',' | xargs printf -- '!**/{%s}/**' | xargs prettier --write works as intended with:

real 0m42.088s
user 0m0.046s
sys 0m0.198s

@eps1lon
Copy link
Member

eps1lon commented Nov 11, 2018

@joshwooding
With "current solution" you mean on master? That produces the following:

$ time yarn prettier
...
real    0m22.638s
user    0m30.348s
sys     0m1.976s

while 2f011dc produces:

$ time yarn prettier
real    0m53.090s
user    0m49.044s
sys     0m14.468s

And looking at the console output it seemingly freezes at some points which is most likely when prettier traverses the directories that are ignored. This was explained in #12564.

I guess this is not much of an issue for CI since we only run on changed anyway but I wouldn't trade perf for "clean code" concerning DX.

@joshwooding
Copy link
Member Author

joshwooding commented Nov 11, 2018

@eps1lon On Windows the current solution on master doesn't ignore the correct files for me. I should have mentioned that earlier, I was talking to @oliviertassinari about it on Gitter

@joshwooding
Copy link
Member Author

joshwooding commented Nov 11, 2018

@eps1lon Can you tell me what cat .eslintignore | tr '\r\n' ' ' | tr -s ' ' ',' | xargs printf -- '!**/{%s}/**' | xargs prettier --write "**/*.{js,d.ts,tsx}" produces?

@eps1lon
Copy link
Member

eps1lon commented Nov 13, 2018

$ cat .eslintignore | tr '\r\n' ' ' | tr -s ' ' ',' | xargs printf -- '!**/{%s}/**' | xargs node_modules/.bin/prettier --write "**/*.{js,d.ts,tsx}"
[error] No matching files. Patterns tried: **/*.{js,d.ts,tsx} !**/{/.git,/.next,/coverage,/docs/export,/examples/create-react-app*/src/serviceWorker.js,/examples/create-react-app-with-flow/flow,/examples/create-react-app-with-flow/flow-typed,/examples/gatsby/public,/flow,/flow-typed,/packages/material-ui-codemod/lib,/packages/material-ui-codemod/src/*/*.test,/packages/material-ui-codemod/src/*/*.test.js,/packages/material-ui-icons/src,/packages/material-ui-icons/fixtures,/packages/material-ui-icons/templateSvgIcon.js,/tmp,build,node_modules,}/** !**/node_modules/** !./node_modules/** !**/.{git,svn,hg}/** !./.{git,svn,hg}/**

@joshwooding
Copy link
Member Author

@eps1lon Can you delete the last empty line in .eslintignore and try again please

@eps1lon
Copy link
Member

eps1lon commented Nov 14, 2018

I guess at this point it's better to use a glob library and programmatically run prettier instead of piping it through all sorts of cli tools. This would improve cross platform DX quite a bit I guess.

@joshwooding
Copy link
Member Author

I thought about programmatically running prettier, I can definitely take a look

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 14, 2018

I believe the React project is programmatically running Prettier.

@eps1lon
Copy link
Member

eps1lon commented Nov 15, 2018

They have their own script that runs prettier programmatically and is able to run on changed files only which is definitely something we might want to add. Not very good DX that you have to format the hole codebase when you're working on a feature branch.

@joshwooding
Copy link
Member Author

Superseded by #13621

@joshwooding joshwooding deleted the simplify-prettier-script branch November 25, 2018 00:55
@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants