-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Pin prettier to a specific version. #10038
Conversation
…t on dev machines and CI.
Should this be necessary when yarn is used on CI? Bug in older yarn (the version used by react is really old)? |
Just curios: Why not pin prettier on |
1.4.4 is the latest, FWIW |
@marvinhagemeister That's also a fine solution, but it would also involve running prettier and updating 40+ files. I don't know if the core team wants to do that right now, so I chose a low impact solution that would stop build failures. |
I don't know really much about yarn, so I'm not sure. I do think, though, that it's pretty clear that we should always pin to just one version of prettier. Since different versions of prettier will create different outputs, and prettier will fail CI, it's critical to pin to the exact version we want to use. |
@@ -76,7 +76,7 @@ | |||
"ncp": "^2.0.0", | |||
"object-assign": "^4.1.1", | |||
"platform": "^1.1.0", | |||
"prettier": "^1.2.2", | |||
"prettier": "1.2.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't yarn.lock
take care of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps? But that won't do anything for folks who are using npm (like me), and the official contribution doc has its instructions in npm, so that's what I thought was supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, fair point.
👍 I'm in favor of this. I can see it being annoying to randomly write a bunch of changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍. I might be missing some nuance with package versioning, but I think this is a great way to avoid confusion and frustration with new contributors. (not that you are new, @aickin :))
Sure, seems fine. We use yarn on the team but let's keep npm working too. |
Yeah prettier doesn't work really well with semver, all the changes we are doing with prettier affect formatting in one way or another, but I don't want to make every release a major bump. I want to give meaning to the version numbers. The real solution is to pin the version (which I truly believe should be the default for npm as well, as we don't want our software to rely on code from the future..., but that's a rant for another time). Lock files also make it safer when used. |
…t on dev machines and CI. (facebook#10038)
…t on dev machines and CI. (facebook#10038)
Right now, prettier's version is
^1.2.2
, which means that a range of versions are acceptable. However, different versions of prettier produce different output. Notably, there are a few dozen files inmaster
that format differently with prettier 1.2.2 and 1.4.0 (the most recent release). This can cause inconsistencies between the formatting on dev machines and CI machines, and even between different CI machines, meaning builds can randomly fail depending which CI machine they get.All this PR does is pins prettier to 1.2.2, which is how the current code in
master
is formatted.(Note this is a follow-on to PR #10037.)