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

Prefer local Prettier when formatting files #905

Merged
merged 6 commits into from Aug 30, 2022
Merged

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Aug 10, 2022

closes #616

it's the best to just take a look at the first commit (the one without formatting changes): 85fa889

@Andarist Andarist requested a review from emmatown August 10, 2022 07:47
@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2022

🦋 Changeset detected

Latest commit: cac4ff2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@changesets/apply-release-plan Minor
@changesets/write Minor
@changesets/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 10, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cac4ff2:

Sandbox Source
Vanilla Configuration

@@ -19,14 +19,25 @@ import prettier from "prettier";
import versionPackage from "./version-package";
import getChangelogEntry from "./get-changelog-entry";

function getPrettierInstance(): typeof prettier {
try {
return require("prettier");
Copy link
Member

@emmatown emmatown Aug 11, 2022

Choose a reason for hiding this comment

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

Could we use createRequire (or maybe something else that has the same behavior if we care about supporting Node versions that don't include createRequire) to resolve it from the repo root? Without that, this doesn't change anything, right?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like require.resolve("prettier", { paths: [projectDir] }) works on more versions so let's probably use that

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ye - this is totally what I had in mind when I started working on this and just forgot it in the rush 🤣

@@ -4,6 +4,17 @@ import prettier from "prettier";
import humanId from "human-id";
import { Changeset } from "@changesets/types";

function getPrettierInstance(): typeof prettier {
try {
return require("prettier");
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to use require.resolve

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

# Conflicts:
#	packages/assemble-release-plan/src/index.ts
#	packages/cli/src/commands/add/index.ts
@Andarist Andarist merged commit c140171 into main Aug 30, 2022
@Andarist Andarist deleted the prefer-local-prettier branch August 30, 2022 06:28
@github-actions github-actions bot mentioned this pull request Aug 30, 2022
aomarks added a commit to lit/lit that referenced this pull request Aug 30, 2022
Previously, weirdness would occur on release because of differences in the Prettier formatting for our CHANGELOG files between our auto-formatter and the formatting that Changesets itself tries to do. This was because Changesets had an old version of Prettier, but our repo had a newer version.

We had a workaround for this where we deleted Changesets' version of Prettier, so that ours would be prefered.

Now that changesets/changesets#905 has landed (fixing changesets/changesets#616), Changesets will prefer our version of Prettier. Plus the version they depend on is now up to date. So we can remove the workaround.
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.

prettier old version
3 participants