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

Upgrade pkg-fetch to 3.5.2 #1914

Merged
merged 2 commits into from
May 2, 2023
Merged

Conversation

dav-is
Copy link
Contributor

@dav-is dav-is commented Apr 20, 2023

To use the newest versions of Node.js, we need to upgrade pkg-fetch (GitHub Release, npm)

@baparham
Copy link
Contributor

This is going to hurt our Mac m1/arm users since this version of pkg-fetch doesn't include a prebuilt binary for mac arm, unfortunately. I'm open to hearing arguments though against bumping, since Mac users have the workaround of building themselves, and/or pinning to the previous version of pkg-fetch

@emmansun
Copy link
Contributor

emmansun commented Apr 21, 2023

Ignore pnpm test for node14 like #1613 or downgrade pnpm ?

https://pnpm.io/installation#compatibility

If we decide to ignore pnpm test for node14, we should just need to change https:/vercel/pkg/blob/c353cc9ce8afba97bb6f5c92f71384498835071b/test/utils.js

module.exports.shouldSkipPnpm = function () {
  const REQUIRED_MAJOR_VERSION = 16; 
  const REQUIRED_MINOR_VERSION = 14;

  const MAJOR_VERSION = parseInt(process.version.match(/v([0-9]+)/)[1], 10);
  const MINOR_VERSION = parseInt(
    process.version.match(/v[0-9]+\.([0-9]+)/)[1],
    10
  );

  const isDisallowedMajor = MAJOR_VERSION < REQUIRED_MAJOR_VERSION;
  const isDisallowedMinor =
    MAJOR_VERSION === REQUIRED_MAJOR_VERSION &&
    MINOR_VERSION < REQUIRED_MINOR_VERSION;
  if (isDisallowedMajor || isDisallowedMinor) {
    const need = `${REQUIRED_MAJOR_VERSION}.${REQUIRED_MINOR_VERSION}`;
    const got = `${MAJOR_VERSION}.${MINOR_VERSION}`;
    console.log(`skiping test as it requires nodejs >= ${need} and got ${got}`);
    return true;
  }

  return false;
};

@robertsLando
Copy link
Contributor

Commented on the other PR regarding the pnpm issue

@emmansun
Copy link
Contributor

emmansun commented Apr 21, 2023

Commented on the other PR regarding the pnpm issue: Shouldn't we check also pnpm version in this check? older pnpm versions also support older node versions

unless we are able to install older version of pnpm for nodejs older version.

@robertsLando
Copy link
Contributor

I don't see any problem with that, mine was just a question, giving that it's just a test util we could consider to just keep last pnpm version supported. I think you could re-open your PR

@justajwolf
Copy link

justajwolf commented Apr 23, 2023

I don't see any problem with that, mine was just a question, giving that it's just a test util we could consider to just keep last pnpm version supported. I think you could re-open your PR

for pkg-fetch version, i think pkg should use version range: "~3.5.2" in deps. to avoid frequently update. you think?

@baparham
Copy link
Contributor

I don't see any problem with that, mine was just a question, giving that it's just a test util we could consider to just keep last pnpm version supported. I think you could re-open your PR

for pkg-fetch version, i think pkg should use version range: "~3.5.2" in deps. to avoid frequently update. you think?

are you suggesting so that end users can let their package managers resolve to more recent yet-to-be released versions of pkg-fetch without needing to roll a new release of pkg?

I don't know the historical reasoning behind why this is hard-pinned to a version. Considering these packages are siblings, I find the risk to be low to allow for semver flexibility. If it were an external package, I would take a hard pass at it, since as we've seen, NOT hard pinning versions opens our end users up to supply chain attacks where one of the dependencies is hijacked and has a new patch version released that all of the sudden everyone starts using. But, like I said, the relationship between pkg and pkg-fetch is a bit closer and can afford some more flexibility considering that risk.

I'm open to it, but I also don't have all the info and would love to hear if there are any suggestions or arguments against loosening the version string.

@robertsLando
Copy link
Contributor

@baparham I think problem is that when we bump a pkg-fetch version there could be new nodejs binaries created for it so a new install of pkg will use those versions instead of the previous ones, we already spoke about this in another issue and could be a problem or not, if there are issues with latest nodejs binaries this could be a problem as you can have 2 installations of pkg one that is working and the other not and you don't understand why

@baparham
Copy link
Contributor

@robertsLando true, it does open users up to unknowingly getting newer versions of pkg-fetch in some circumstances, however, isn't this what lock files are meant to prevent (i.e. yarn.lock or package-lock.json)? Presumably users who forego the protections of a lock file understand the consequences of dependency version volatility. Are there cases where a user might still get a new version of pkg-fetch even if they have a lock file maybe?

@robertsLando
Copy link
Contributor

Are there cases where a user might still get a new version of pkg-fetch even if they have a lock file maybe

I don't think so

@justajwolf
Copy link

@baparham I think problem is that when we bump a pkg-fetch version there could be new nodejs binaries created for it so a new install of pkg will use those versions instead of the previous ones, we already spoke about this in another issue and could be a problem or not, if there are issues with latest nodejs binaries this could be a problem as you can have 2 installations of pkg one that is working and the other not and you don't understand why

Me,too. I also approve of this.

@justajwolf
Copy link

@robertsLando true, it does open users up to unknowingly getting newer versions of pkg-fetch in some circumstances, however, isn't this what lock files are meant to prevent (i.e. yarn.lock or package-lock.json)? Presumably users who forego the protections of a lock file understand the consequences of dependency version volatility. Are there cases where a user might still get a new version of pkg-fetch even if they have a lock file maybe?

I think that pkg-fetch is backward compatibility range patch versions. so pkg deps should use semver version limit major and minor, but hard-pinned to a version. you think?

Besides this,i think that pkg must be relate to pkg-fetch. pkg-fetch is sub package of the pkg.

@baparham
Copy link
Contributor

please rebase, node 14 test should be fixed on main now

@baparham
Copy link
Contributor

BTW, I think the decision will be to keep the hard pinned version (i.e. "3.5.2"), as this PR has it. When we release pkg-fetch it makes just as much sense to release an associated pkg to keep them in lock-step.

@justajwolf
Copy link

BTW, I think the decision will be to keep the hard pinned version (i.e. "3.5.2"), as this PR has it. When we release pkg-fetch it makes just as much sense to release an associated pkg to keep them in lock-step.

ok, i approve this decision. when pkg-fetch version to update, pkg version should also update version?

Copy link
Contributor

@emmansun emmansun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebase after node14 test fix. @baparham, pls kindly help to review it.

@emmansun
Copy link
Contributor

emmansun commented May 2, 2023

Hi @robertsLando , pls merge this PR, thx!

@baparham baparham merged commit 76010f6 into vercel:main May 2, 2023
@dav-is dav-is deleted the upgrade-pkg-fetch-3-5 branch May 2, 2023 16:34
pcnate pushed a commit to pcnate/pkg that referenced this pull request Aug 15, 2023
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