-
Notifications
You must be signed in to change notification settings - Fork 27
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: support prettier v3 #195
Conversation
node: [12.x, 14.x, 16.x, 18.x] | ||
prettier: [2.1, 2.2, 2.4, latest] | ||
node: [14.x, 16.x, 18.x, 20.x] | ||
prettier: [3.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not acceptable, we can't remove compatibility of old version of prettier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be hard to maintain both sync and async version of plugin. [email protected]
is stable enough to work with prettier@2
, so maybe we can release a new major version 1.0.0
that will support only prettier@3
and add to README something like:
This plugin is compatible with
[email protected]
, but if you want to use this plugin with[email protected]
, then you can install it withnpm i prettier-plugin-jsdoc@prettier-v2
Prettier v2 will no longer receive updates and the plugin ecosystem is already moving towards prettier v3 adoption
The official plugins already dropped prettier@2
support:
@prettier/plugin-ruby
- prettier/plugin-ruby@47a68f2@prettier/plugin-xml
- prettier/plugin-xml@8a01168
Also migration to ESM will allow us not to include mdast-util-from-markdown
to the bundle and significantly reduce package size. Otherwise if we will include both sync and async versions, the package size will only grow
package.json
Outdated
"typescript": "^4.4.4" | ||
}, | ||
"peerDependencies": { | ||
"prettier": ">=2.1.2" | ||
"prettier": ">=3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not make the same mistake as your predecessors. The peer dependency should be declared with ^
or ~
, not >=
❌
Also, it would be fair to correct the peer dependency on prettier of the current 0.4 line of this plugin to read ^2.1.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, thanks! I changed it to ^3.0.0
@hosseinmd Hi, thanks for supporting prettier v3! How about creating a new release? :) |
to get Prettier v3 compat hosseinmd/prettier-plugin-jsdoc#195
Looking forward to a release of this. I also nominate @auvred as an additional maintainer for this important project. |
What version you are suggesting to release this change? It seems like a breaking change, so we should move to 1.0.0 |
In addition, need to be mention in README, compatible versions with prettier v3 and v2 |
Maybe 1.0.0 / 0.5.0 (major is 0 means this package is not stable yet) ? |
I think it is ok to release major version. |
Your project but in my experience, it's still not stable. Per https://semver.org/, Until you release 1.0 there is no obligation to bump the major version on breaking changes. Once you do this, any breaking change will require a major version bump. I would request that you release as 0.5.0 so as to maintain flexibility in designing the options and their defaults. |
Will every change in the formatting output constitute a breaking change in the future? |
No, some formatting output changes will be necessary to fix bugs. But changes in intent (such as flipping a default or removing an option) will be breaking changes. And ofc anything that throws an error. |
In my opinion, this is a breaking change for someone who doesn't want to move to prettier v3, so if anybody installed this by caret ^0.4.x might get 0.5.0 then break. We should prevent them to get this version, so we should release major version |
@hosseinmd IMO this package's major version is 0, which means it may introduce breaking changes in any minor version change. But I don't mind whether you release a major or minor! :) |
So, I released v1.0.0. Thank you. |
Closes #194
This PR includes a lot of changes, I tried to make them as few as possible, so I didn't run
prettier -w
, to prevent formatting that might complicate the PR review.Checklist:
Drop Node.js 12 support, add Node.js 20 to test matrix
Prettier dropped node 12 in 3.0.0
Change parser imports according to new prettier's files structure
Migrate to new async api
Migrate to ESM
Prettier added support for ESM plugins. Also please read this
Migrate from
ts-jest
tojest-light-runner
+ts-node
To run jest test with new prettier version we need to run node with
--experimental-vm-modules
(references: jest docs,--experimental-vm-modules
,node:vm
)Based on my observations it's not stable yet. I tried to run it on node 14 in Github Actions. It fails on
Segmentation fault
. References:import()
instead of callingimportModuleDynamically
nodejs/node#35889So I decided to switch to
jest-light-runner
(it's used by prettier itself). It runs without V8 VM, so it's faster and more stable for nowAlso I dropped
babel-jest
and other babel-related dependencies, because they are no longer neededAdd
"type": "module"
and"exports"
topackage.json
. Set"engines": {"node": ">=14.13.1 || >=16.0.0"}
Set
esm
as output format inrollup.config.js
This allowed to revert this commit 9fa0474 and achieve smaller package size:
If I missed something, please correct me.
P.S.
It'd be nice to update all the dependencies, because they're very outdated... I can do this in this or another PR
It seems that the
umd
build is incorrect, because when I pastto the webpage, it throws this
TODO after review:
prettier -w