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

feat: make the cli work with/without prettier-eslint peer #438

Merged
merged 1 commit into from Aug 14, 2022
Merged

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Aug 12, 2022

What:

close #144, close #436

BREAKING CHANGE: bump all upgradable (dev)Dependencies except pure ESM

(WTF semantic-release...)

We must be careful on merging...

BREAKING CHANGE: token must be in the footer of the commit (which is stupid again and again)

Why:

close #144

How:

mark prettier-eslint as optional peer dependency, and fallback to internal @prettier/eslint which actually pointing to prettier-eslint

@JounQin JounQin added enhancement dependencies Pull requests that update a dependency file labels Aug 12, 2022
@JounQin JounQin requested a review from kylemh August 12, 2022 15:38
@JounQin JounQin self-assigned this Aug 12, 2022
@JounQin JounQin changed the title feat: make the cli work with/without prettier-eslint peer feat: make the cli work with/without prettier-eslint peer Aug 12, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 12, 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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2022

Codecov Report

Merging #438 (6b41988) into master (58101c1) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 6b41988 differs from pull request most recent head 2a7fb4d. Consider uploading reports for the commit 2a7fb4d to get more accurate results

@@            Coverage Diff            @@
##            master      #438   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         4    +1     
  Lines          161       163    +2     
  Branches        27        27           
=========================================
+ Hits           161       163    +2     
Impacted Files Coverage Δ
src/format-files.js 100.00% <ø> (ø)
src/prettier-eslint.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

.nvmrc Outdated Show resolved Hide resolved
src/prettier-eslint.js Show resolved Hide resolved
src/prettier-eslint.js Show resolved Hide resolved
@idahogurl
Copy link
Collaborator

@JounQin FYI, the release didn't work. semantic-release determined it didn't need to version bump. https://github.com/prettier/prettier-eslint-cli/runs/7808657314?check_suite_focus=true

[3:01:49 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: feat!: bump all upgradable (dev)Dependencies except pure ESM (#437)
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  The commit should not trigger a release

@JounQin
Copy link
Member Author

JounQin commented Aug 12, 2022

@JounQin FYI, the release didn't work. semantic-release determined it didn't need to version bump. https://github.com/prettier/prettier-eslint-cli/runs/7808657314?check_suite_focus=true

[3:01:49 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: feat!: bump all upgradable (dev)Dependencies except pure ESM (#437)
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  The commit should not trigger a release

That's why I don't like it, I'd prefer changesets for releasing.

Any idea how can I fix? Or can we migrate to changesets?

cc @kylemh

@kylemh
Copy link
Collaborator

kylemh commented Aug 12, 2022

changesets wouldn't have magically resolved our issue either. In this situation, there's TECHNICALLY no breaking change for consumers because there's no interface change. That being said, I don't mind choosing a major release because we've changed how prettier-eslint resolves.

Instead of migrating to changesets we should figure out how to force a publish with a manually defined version for this one instance.

@JounQin
Copy link
Member Author

JounQin commented Aug 12, 2022

changesets wouldn't have magically resolved our issue either. In this situation, there's TECHNICALLY no breaking change for consumers because there's no interface change. That being said, I don't mind choosing a major release because we've changed how prettier-eslint resolves.

Instead of migrating to changesets we should figure out how to force a publish with a manually defined version for this one instance.

changesets is based on changeset file, not relying on the commit message only. So there will be no issue if we migrate to changesets, because we can always add changeset file later before releasing.

Take prettier/eslint-plugin-prettier@7bd70b6#diff-8a555112659f40251b1022a16f7f984287f501c43766c71c0bfd2c2f3ef6ffa1R2 as an example.

@kylemh
Copy link
Collaborator

kylemh commented Aug 12, 2022

Oh damn. I WAS going to say: I'm not at all worried to switch to changesets. I love the tool! I'm simply saying this isn't really a good reason to switch away from semantic-release; however, I just googled and saw this:

You can trigger a release by pushing to your Git repository. You deliberately cannot trigger a specific version release, because this is the whole point of semantic-release.

That seems silly.


Okay, I don't know. Interested to hear what other say. The tool is technically correct. This isn't a major version change, but - in my opinion - versioning is best served as a communication tool with consumers. Making it a strict reference to changes in interface seems like losing out on an opportunity to be really responsible to our users.

@JounQin
Copy link
Member Author

JounQin commented Aug 12, 2022

That seems silly.

Stupid, actually. 😁

Okay, I don't know. Interested to hear what other say. The tool is technically correct. This isn't a major version change, but - in my opinion - versioning is best served as a communication tool with consumers. Making it a strict reference to changes in interface seems like losing out on an opportunity to be really responsible to our users.

That's the point why I want to migrate to changesets! Right? 🤝

But to make it not a blocker, I'd like to mark a BREAKING CHANGE: commit in another PR.

@kylemh
Copy link
Collaborator

kylemh commented Aug 12, 2022

I like the non-blocking strategy personally, but will defer to others on that.

Alternative to changesets would be https://github.com/intuit/auto so that we can simply label changes whenever we want.

Either works well enough, imo.

@JounQin
Copy link
Member Author

JounQin commented Aug 13, 2022

I like the non-blocking strategy personally, but will defer to others on that.

Yep, I'll raise a new PR requesting review later.

Alternative to changesets would be intuit/auto so that we can simply label changes whenever we want.

Either works well enough, imo.

I've been using changesets for a long time:

  1. we can define a custom/preset changelog format, including thanks mentions for contributors
  2. We can publish snapshot versions easily for testing before releasing via changesets-snapshot-action, although CodeSandbox CI has been doing this job

I'm not so familiar with https://github.com/intuit/auto.

close #144

BREAKING CHANGE: bump all upgradable (dev)Dependencies except pure ESM
@idahogurl
Copy link
Collaborator

My only concern with making it a peer dependency is that what if we make a breaking change where the CLI only works with pretter-eslint at some version.

@JounQin
Copy link
Member Author

JounQin commented Aug 14, 2022

My only concern with making it a peer dependency is that what if we make a breaking change where the CLI only works with pretter-eslint at some version.

We can change the version range of peer.

@idahogurl
Copy link
Collaborator

Oh as for semantic-release vs changesets vs that Intuit one. I really don't have a preference. I use semantic-release for my VS Code extension and ended up doing what @kylemh is talking about by setting things. https://github.com/idahogurl/vs-code-prettier-eslint/blob/396efd13e65eb2cc624f9e354c0434ce504e9ea7/release.config.js#L8-L10

@JounQin
Copy link
Member Author

JounQin commented Aug 14, 2022

Let's skip changesets in this PR and continue at #441

@idahogurl Are we good to merge this PR?

@JounQin JounQin requested a review from idahogurl August 14, 2022 02:28
@JounQin JounQin merged commit 39c38b5 into master Aug 14, 2022
@JounQin JounQin deleted the feat/peer branch August 14, 2022 03:01
@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v7] bump dependencies
4 participants