-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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(plugin-npm2yarn): Add Bun to default tabs conversions #10953
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
Thanks Ok to do that but I'm going to consider it a feat, not a fix. It's a behavior change that users upgrading must be aware of, and documented properly in our release notes. I even wonder if we shouldn't keep it for v4 (soon), because it's not critical and may annoy some users upgrading that would now need to disable this conversion. See for example: electron/website#421 Any thought @Josh-Cena ? |
Yeah, good points. I debated prefixing the PR description with "fix" vs "feat", and only went with "fix" because the README conflicted with the actual behavior. A major version bump would be a good time to make the change. |
Yeah, let's delay this to v4, it's safer, not too far, and there's a good enough workaround until then. |
I don't have thoughts wrt to v3 or v4. I don't think it's major enough, actually—it's a normal feature enhancement. I don't really know what users expect when they use this feature though. |
I don't have strong feelings about it either, but FWIW as a user of the feature I would assume the default meant "show all the reasonably popular ones" (which includes Bun at this point). I doubt many people would assume the default meant "show all the popular ones as of the last major version bump of this Docusaurus plugin," and anyone needing to customize the list would have already done that anyway. |
Ok let's merge this for v3.8 then 👍 It's easy to revert if needed and shouldn't really harm anyone |
It's customizable anyway—we just need to point to the option to restore previous behavior in the release notes |
Pre-flight checklist
Motivation
Bun is mentioned as being one of the default converters in
packages/docusaurus-remark-plugin-npm2yarn/README.md
, but it wasn't actually implemented as a default inpackages/docusaurus-remark-plugin-npm2yarn/src/index.ts
.Bun is probably popular enough by now that it warrants inclusion as a default.
I also updated the README for the package to show the default converters as an actual array value and updated the example screenshot with pnpm and Bun.
Test Plan
Updated
packages/docusaurus-remark-plugin-npm2yarn/src/__tests__/__snapshots__/index.test.ts.snap
.I wasn't able to run the tests locally, so I updated the snapshot manually for the first commit. I can fix the snapshot if the tests fail in CI.Fixed.Test links
Deploy preview: https://deploy-preview-10953--docusaurus-2.netlify.app/docs/installation/#build
Related issues/PRs