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

Update prettier to v3 #12206

Merged
merged 4 commits into from
Sep 27, 2024
Merged

Update prettier to v3 #12206

merged 4 commits into from
Sep 27, 2024

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Sep 18, 2024

Description

Updates prettier to v3.3.0 which crosses the major version 3 breaking changes

This is a very big pr touching nearly all our source files but all the changes are basically identical.

Key changes:

  • Trailing commas are now always included (a very good change IMO), this is a majority of the changes
  • greatly improved line wrapping logic for readability (see below)
  • prefer lowercase doctype over DOCTYPE
// before 3.0 it preferred parens and brackets
const tileset = await Cesium.Cesium3DTileset.fromIonAssetId(
  1
);

// after 3.0 it actually somewhat uses context, much more readable imo
const tileset =
  await Cesium.Cesium3DTileset.fromIonAssetId(1);

I also added a new .prettierrc specifically for the Sandcastle Gallery to allow a slightly wider line with to account for the indentation that's removed when sandcastles are actually rendered. This should produce code that matches prettier's default 80 when in the browser editor.

When we decide to merge this I'll add some new git tags and post a guide on how to update other branches easier.

Issue number and link

Fixes #12172

Testing plan

  • Run npm run prettier and make sure there are no changed files
    • or just run npm run prettier-check and make sure it doesn't fail
  • make sure specs still pass
  • functionally everything should be the same so primarily review the files themselves.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@jjspace jjspace mentioned this pull request Sep 20, 2024
6 tasks
@ggetz
Copy link
Contributor

ggetz commented Sep 20, 2024

Thanks @jjspace! As we discussed offline, could you please make separate commits for 1) the package.json and other configuration updates and 2) the result of running the prettier command? This would help make everything easier to review. 🙏

@ggetz ggetz self-assigned this Sep 20, 2024
@ggetz
Copy link
Contributor

ggetz commented Sep 25, 2024

Sorry for the delay here @jjspace. If you merge in main this should be good to go.

@ggetz
Copy link
Contributor

ggetz commented Sep 25, 2024

Please mention me when this is ready to go, so I can merge ASAP.

@jjspace
Copy link
Contributor Author

jjspace commented Sep 26, 2024

@ggetz this should be good to go. The previous time we did a huge prettier change we provided these instructions

I've tagged 21acce1 as pre-prettier-v3 and 09a719b as post-prettier-v3 locally. If these seem correct to you I can push those tags up.

The updated instructions would be as follows:

  1. Fetch changes from the upstream git fetch upstream. Replace upstream with the name of your git remote that points to the official CesiumGS/cesium repo.
  2. Create a new branch for your merge: git checkout -b whatever-merge. This is not strictly necessary, but it makes it easier to start over if something goes wrong in this process.
  3. Merge the last commit before Prettier v3 into your branch: git merge pre-prettier-v3 . Resolve any conflicts as you usually do.
  4. Commit the merge above.
  5. Merge the Prettier formatting changes, accepting your own changes in all cases: git merge post-prettier-v3 -Xours --no-commit Accepting yours is safe because this commit only changes formatting. Do NOT commit yet.
  6. Run npm install to make sure you're on the latest version of prettier and then run npm run prettier on the result of the merge before committing it
  7. Commit these changes (which is technically a merge commit).
  8. Merge main into your branch in order to pick up any changes in main that happened after Prettier was introduced: git merge upstream/main . You can also merge a release tag here instead of main, if desired. Resolve any conflicts as normal and commit; there shouldn’t be any Prettier-related conflicts.
  9. Once you’re satisfied with the result, merge it back into your original branch, if you created a new one in step 2: git checkout my-original-branch && git merge whatever-merge
  10. You may need to restart editors like VSCode to pick up the new version of prettier if they give you any issues with auto-formatting.

@ggetz
Copy link
Contributor

ggetz commented Sep 27, 2024

Awesome, thanks @jjspace! Please post those instructions to the forum. It would also be kind to let developers with active PRs know and give them a link to the forum thread.

@ggetz ggetz merged commit 3082d3e into main Sep 27, 2024
9 checks passed
@ggetz ggetz deleted the update-prettier-v3 branch September 27, 2024 13:13
@jjhembd
Copy link
Contributor

jjhembd commented Sep 27, 2024

@jjspace thanks for those clear instructions! I'm not seeing the tags pre-prettier-v3 and post-prettier-v3. Can you please push those up?

@jjspace
Copy link
Contributor Author

jjspace commented Sep 27, 2024

@jjhembd pushed!

@ggetz ggetz mentioned this pull request Sep 27, 2024
6 tasks
@jjhembd
Copy link
Contributor

jjhembd commented Sep 27, 2024

@jjspace one suggestion on step 5:

  1. Merge the Prettier formatting changes, accepting your own changes in any chunk with a conflict: git merge post-prettier-v3 -Xours --no-commit. Accepting yours is safe because this commit only changes formatting. Do NOT commit yet.

The -Xours keeps the post-prettier-v3 code where possible.

--strategy ours actually discards all the post-prettier-v3 code. In theory it should work because the next step (running prettier) should re-create what was in post-prettier-v3. But then npm run prettier ends up changing ALL the files, which is hard to QC, and easy to mess up if prettier gets stuck somewhere.

When I was merging into #12188, the -Xours option meant that the npm run prettier step only changed the files that were affected by my PR, which was a lot easier to review.

@jjspace
Copy link
Contributor Author

jjspace commented Sep 27, 2024

Thank you @jjhembd!! I confused trying this out on another branch and that change helped a lot. I've updated the steps accordingly.

I've also posted this to the forum now https://community.cesium.com/t/cesiumjs-updated-prettier-to-version-3-3-3/35473

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.

Update prettier
3 participants