docs(node-resolve): Update rollup requirement in readme #1266
Conversation
|
Node 10 should be correct. At least when I make PRs, I try to keep package.json as the source of truth, but I may have missed the Readme. |
|
It's a bit hypothetical if tests aren't run on that version, though a quick check with the last 10, 10.24.1 does still succeed quite nicely! At minimum, rollup declares it needs 10 as well, so we should at least match that. |
simonbuchan
force-pushed
the
patch-1
branch
2 times, most recently
from
ed3d354 to
a15dd8a
Compare
simonbuchan
changed the title
|
Updated the node version, and fixed the PR and commit prefix while I was there (not sure if the commit message matters, looks like it squashes) |
packages/node-resolve/README.md
Outdated
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| ## Requirements | |||
|
|
|||
| This plugin requires an [LTS](https://github.com/nodejs/Release) Node version (v8.0.0+) and Rollup v1.20.0+. | |||
| This plugin requires an [LTS](https://github.com/nodejs/Release) Node version (v10.0.0+) and Rollup 2.78.0+. | |||
There was a problem hiding this comment.
I know this contradicts what @lukastaegert had said, but we only support actual LTS versions of Node. https://nodejs.org/en/about/releases/ Right now, that's v14+
I'll fast-follow this PR with a package.json update.
packages/node-resolve/README.md
Outdated
| @@ -221,7 +214,7 @@ export default ({ | |||
| }) | |||
| ``` | |||
|
|
|||
| ## Resolving Require Statements | |||
| ## Resolving require statements | |||
There was a problem hiding this comment.
please try not to reformat. this change needs to be reverted please
There was a problem hiding this comment.
Little confused how that happened! Definitely wasn't deliberate.
packages/node-resolve/README.md
Outdated
| Default: `[]` | ||
|
|
||
| A list of absolute paths to additional locations to search for modules. [This is analogous to setting the `NODE_PATH` environment variable for node](https://nodejs.org/api/modules.html#loading-from-the-global-folders). | ||
| One or more directories in which to recursively look for modules. |
There was a problem hiding this comment.
I don't see this change mentioned in the PR description or the linked issues. Is there a reason that the modulePaths option was removed?
There was a problem hiding this comment.
Similar: I didn't deliberately do this. I can't figure why either the GitHub editor or Webstorm would have made these changes?
Ugh. Too late for me today, I'll fix it tomorrow.
simonbuchan
force-pushed
the
patch-1
branch
from
a15dd8a to
e0a95e4
Compare
|
Sorry about the confusion, looks like there was some dumb rebase issue with my fork being out of date, which stomped on the changes from 12d87a4 Raised the node version to 14 too. |
simonbuchan
force-pushed
the
patch-1
branch
from
e0a95e4 to
429710b
Compare
|
Pay no attention to me screwing up the rollup version last time... sigh. My most difficult PR has been updating some numbers in a README file. FML. |
|
Haha no worries. Looks like our automation is blocked by some linting errors that snuck in (we're not sure how), but #1270 fixes this. |
|
Thanks to your PR, I made sure to update the requirements in the readme files of all plugins in the Rollup 3 compatibility PRs I just created. This may supersede your PR, see #1288, but it was still valuable to point us to this possible discrepancy. |
|
Eh, I'm fine with this being closed, assuming there's no particular reason to delay the other one. Despite my incompetence here, it's not exactly a lot of lost work 😉 |
Rollup Plugin Name:
node-resolveThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
Minimum rollup version was increased in #1245. I'll wait on someone figuring out the actual minimum node version, I doubt even the updated engine requirement of 10 mentioned in #1222 is correct.