Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

Support newer node versions which changed internalModuleReadJSON #1059

Merged
merged 2 commits into from
Mar 24, 2021

Conversation

xanonid
Copy link
Contributor

@xanonid xanonid commented Mar 8, 2021

The PR is inspired by https://github.com/vercel/pkg-fetch/pull105#issuecomment-757116069

This is a pre-requisite for updating to newer Node.js version (currently supports only v12, v14 and for future usage >=v15). The change should preserver backward-compatibility.

Related: #1008 https://github.com/vercel/pkg-fetch/issues/114

@leerob
Copy link
Member

leerob commented Mar 8, 2021

The comment you linked to isn't working. Is there any issue or other PR this relates to here?

Edit: Nvm, I think there's enough context in the other linked issue. There's likely more to close in this repo.

@xanonid
Copy link
Contributor Author

xanonid commented Mar 8, 2021

The issue fixed by this PR is mentioned in vercel/pkg-fetch#105 - the PR itself is prerequisite for vercel/pkg-fetch#115

@jesec
Copy link
Contributor

jesec commented Mar 23, 2021

Thanks for the PR and good job making it backwards-compatible.

Indeed, this change is needed for newer versions of Node.js. I picked @alexk111 ‘s similar patch to my fork and it is working perfectly fine in production so far: jesec@44fb852.

That revision is not backwards compatible, though.

I will merge this PR once I tested it.

BTW: As you apparently got the inspiration from @alexk111 , I think it would be fair to add them as co-author or split the change into two. (one for fix, one for backwards compatibility) I can help you to do that.

@xanonid
Copy link
Contributor Author

xanonid commented Mar 23, 2021

@jesec Feel free to split the commit as you think it is best.

jesec added a commit to vercel/pkg-fetch that referenced this pull request Mar 24, 2021
@jesec jesec force-pushed the support-newer-internalModuleReadJSON branch from 367330b to 1801a95 Compare March 24, 2021 07:08
@jesec jesec force-pushed the support-newer-internalModuleReadJSON branch from 1801a95 to e4be7cd Compare March 24, 2021 07:09
jesec
jesec previously approved these changes Mar 24, 2021
@jesec
Copy link
Contributor

jesec commented Mar 24, 2021

Note to maintainers:

"Rebase and merge" here, so authorship of different commits can be preserved.

jesec pushed a commit to vercel/pkg-fetch that referenced this pull request Mar 24, 2021
@jesec jesec dismissed their stale review March 24, 2021 11:49

Hold for now. returnArray is somehow true with Node 10.21.0.

@jesec jesec force-pushed the support-newer-internalModuleReadJSON branch from e4be7cd to 602ceb8 Compare March 24, 2021 11:55
@jesec
Copy link
Contributor

jesec commented Mar 24, 2021

Changes made:

@@ -32,8 +32,6 @@ var removeUplevels = common.removeUplevels;
 var FLAG_ENABLE_PROJECT = false;
 var NODE_VERSION_MAJOR = process.version.match(/^v(\d+)/)[1] | 0;
 var NODE_VERSION_MINOR = process.version.match(/^v\d+.(\d+)/)[1] | 0;
-var NODE_VERSION_PATCH = process.version.match(/^v\d+.\d+.(\d+)/)[1] | 0;
-

 // /////////////////////////////////////////////////////////////////
 // ENTRYPOINT //////////////////////////////////////////////////////
@@ -1201,13 +1199,10 @@ function payloadFileSync (pointer) {
     // For newer node versions (after https://github.com/nodejs/node/pull/33229 ):
     // Returns an array [string, boolean].
     //
-    var returnArray = (NODE_VERSION_MAJOR === 12 &&
-                        (NODE_VERSION_MINOR === 18 && NODE_VERSION_PATCH >= 3) ||
-                        NODE_VERSION_MINOR > 18) ||
+    var returnArray = (NODE_VERSION_MAJOR === 12 && NODE_VERSION_MINOR >= 19) ||
                       (NODE_VERSION_MAJOR === 14 && NODE_VERSION_MINOR >= 5) ||
                       (NODE_VERSION_MAJOR >= 15);

Fixes the conditional by simplifying it to ">= 12.19.x". As we are not going to ship =12.18.x and IMO better readability is more important, the VERSION_PATCH part is discarded.

@jesec jesec merged commit 9598890 into vercel:master Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants