-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: release a version for nx 17 & 18 #1344
base: main
Are you sure you want to change the base?
Conversation
packages/nx-gradle/package.json
Outdated
@@ -40,7 +40,6 @@ | |||
"@jnxplus/common": "1.12.2", | |||
"tslib": "^2.6.2", | |||
"@nx/devkit": ">=17.0.0", | |||
"smol-toml": "^1.1.3", | |||
"nx": ">=17.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.
Maybe a suggestion to add a peerDependency specifying which versions of NX you officially support. For instance, for the 0.x versions you only support NX >= 18.3.5, but this is not explicit. With peer dependencies users of your library will get warnings/errors when they use a version of NX that you don't officially support.
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.
@PieterVanEde 0.50.0 is working good with NX 17, I will add peerDependency
in a future version maybe 0.50.1
https://github.com/khalilou88/jnxplus/actions/runs/11362843593/job/31605536086
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.
@PieterVanEde check version 0.50.1
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.
That looks good to me! But in several places I saw you mentioning 18.3.5 explicitly, but this peerDependency also allows for instance 18.0.1.
I know that older 18.x versions work correctly, but in one of my bug reports I received a comment that my NX version was not supported. So I think the peerDependency should reflect what you officially want to support. If people then deviate, they know that their mileage may vary.
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.
@PieterVanEde My idea was to always use the latest version of 18. Because Other version of 18 contains bugs and maybe breaking changes.
I think version 19.2.0 introduce a breaking change when they added workspaceDataDirectory
.
I tested 0.50.1 with version 17 and 18 and it's working. Yesterday I just tested with version nx 18.3.5
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 see, but I would perhaps indeed only update peerDependencies if there is a breaking change or known bug in an NX version that does not make you want to support it.
Otherwise, if a user of your library has a reason to use an outdated version of NX, you might not want to restrict it's usage because of reasons beyond those that create problems for you plugins.
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 will try to do better next time. Now jnxplus fully support NX LTS versions.
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.
Then I think I wrongly conveyed my message: I actually meant that your decision to specify the range of 17 and 18 is the right thing to do. I meant more to say, that it is good that you did not specify 18.3.5 just because it has the latest bugfixes, but that it would be correct to specify minor versions if there is a breaking change or bugfix that you depend on.
So keep up the great work!
5a6f671
to
3393ce9
Compare
No description provided.