search

rollup / plugins

🍣 The one-stop shop for official Rollup plugins
MIT License
3.63k stars 585 forks source link

[@rollup/plugin-node-resolve] Fix required versions of Node/Rollup in README + include these breaking changes in CHANGELOG #1222

Open 0xdevalias opened 2 years ago

0xdevalias commented 2 years ago

Documentation Is:

Please Explain in Detail...

Should the README be updated to align with this then? As currently it says:

This plugin requires an LTS Node version (v8.0.0+) and Rollup v1.20.0+.

Yet package.json suggests that it actually requires Node >= 10.0.0 and peerDependencies of "rollup": "^2.42.0"

Looking at the CHANGELOG, i'm not sure this was marked as a breaking change anywhere either was it?

Originally posted by @0xdevalias in https://github.com/rollup/plugins/issues/1023#issuecomment-1182790933

Your Proposal for Changes

The README and CHANGELOG should be kept up to date with breaking changes introduced in package versions, in end-user friendly descriptive language.

0xdevalias commented 2 years ago

It looks like node 10.x was required since https://github.com/rollup/plugins/commit/e6324695220a8e4c6e34e3537df32bf6a7814041 (31 Jul 2020)

https://github.com/rollup/plugins/blob/master/packages/node-resolve/CHANGELOG.md#v900

v9.0.0 Breaking Changes

  • chore: update dependencies (e632469)

It looks like rollup 2.x was required since https://github.com/rollup/plugins/commit/3a543dfd9142f936b0157893580c25f4cb729b0f (1 May 2021)

https://github.com/rollup/plugins/blob/master/packages/node-resolve/CHANGELOG.md#v1300

v13.0.0 Breaking Changes

  • fix!: mark module as external if resolved module is external (#799)

While technically these were both included in the CHANGELOG, they weren't particularly user friendly/obvious in their intent. In contrast, an earlier 'breaking changes' was very clear:

6.0.0

  • Breaking: Minimum compatible Rollup version is 1.20.0
  • Breaking: Minimum supported Node version is 8.0.0
agilgur5 commented 2 years ago

Similar story for https://github.com/rollup/plugins/pull/1245, which requires Rollup 2.78.0 now, but doesn't mention this in the CHANGELOG.md entry besides saying it's "breaking" in some way.

The changelog is auto-generated, which encounters some issues where human intervention / readability is required, like in this case.

I was looking to fix these (and the lack of hyperlinks to PRs in the Markdown) at one point in time too, but rather than a simple CHANGELOG.md change, this would require changes to the automated script and more. The script doesn't really have a feature for manual notes, so this would likely require a new dependency etc. Basically it's much bigger in scope than originally thought. As such it probably requires some discussion with maintainers before actually proceeding with an implementation. So a simple docs change PR wouldn't cut it 😕

shellscape commented 2 years ago

Folks, I really don't mean for this to be curt; if the lack of a hyperlink is stopping you from following the issue number, you may want to consider slowing down a tad, use the problem solving skills that got you into the industry to discern how to view the PR or issue number. In terms of blockers, that's the softest one I've ever seen.

We'll update the script so it creates hyperlinks, but do try and take initiative to learn the platform you're using.