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

chore: update and pinned prettier to version 3.4.2 and fix ci #7111

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

qupig
Copy link
Contributor

@qupig qupig commented Dec 12, 2024

According to the Prettier documentation, we should always install an exact version:

Install an exact version of Prettier locally in your project. This makes sure that everyone in the project gets the exact same version of Prettier. Even a patch release of Prettier can result in slightly different formatting, so you wouldn’t want different team members using different versions and formatting each other’s changes back and forth.

That is, in package.json it should look like "prettier": "3.0.3" instead of "prettier": "^3.0.3".

Don’t forget the --save-exact parameter when installing or updating the prettier:

npm install --save-dev --save-exact prettier@3.4.2

Otherwise, following the Development workflow, one can easily get different prettier versions and end up with CI failing, and this is often a source of confusion.

Therefore, this PR mainly pinned the version of prettier and updates it to the latest version v3.4.2 by the way.

It's worth noting that I added the commit hash of the actual formatted code in .git-blame-ignore-revs to ignore non-actual code changes for git blame.

  • It is recommended that future prettier version updates take the same action
  • So note that this PR should NOT be squash/rebase merge, otherwise the commit hash ​​will not match
    • Or you could revise the commit hash in .git-blame-ignore-revs after merging

The original CI process uses the third-party workflow actionsx/prettier@v3 with a fixed version of prettier (currently seems to be 3.0), which is another cause of confusion.

We should always use the version specified in package.json, and always maintain version numbers in one place.

The speed difference is insignificant here, and it's not a shortcoming in the overall CI process.

@qupig qupig requested a review from a team as a code owner December 12, 2024 20:28
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Ahhh that explains why that block kept getting formatted differently. Thank you for fixing this!

@code-asher code-asher enabled auto-merge December 12, 2024 21:31
@qupig
Copy link
Contributor Author

qupig commented Dec 12, 2024

It seems there are some other dependencies in there that need to be updated, unrelated to this PR, but preventing the merge.

https://github.com/coder/code-server/actions/runs/12304253477/job/34343664857

auto-merge was automatically disabled December 13, 2024 00:07

Head branch was pushed to by a user without write access

@qupig
Copy link
Contributor Author

qupig commented Dec 13, 2024

Try to using npm audit fix to auto fix and update express from 5.0.0-beta.3 to the release version ^5.0.1.

But it looks like E2E Tests are failing: https://github.com/qupig/code-server/actions/runs/12306978786

BTW, I looked at those failing video artifacts, but it seems like every video is blank.

I'm not familiar with those parts, maybe you could check the changes v5.0.0-beta.3...5.0.1 and release notes.

@code-asher
Copy link
Member

code-asher commented Dec 13, 2024

Hey, sorry about that, yeah we have not updated Express yet because doing so causes it to completely fail, and as you noticed all you get is a blank screen (I think all of the http requests error or time out). I thought the check was optional, but I suppose it must not be if the auto-merge failed. Edit: hm it is optional, not sure why the auto-merge did not go through.

@code-asher
Copy link
Member

I will drop that last commit and force merge

@code-asher code-asher merged commit 99e1f63 into coder:main Dec 13, 2024
13 of 14 checks passed
@code-asher
Copy link
Member

Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants