vercel / pkg

Package your Node.js project into an executable
https://npmjs.com/pkg
MIT License
24.33k stars 1.02k forks source link

Upgrade pkg-fetch to 3.5.2 #1914

Closed dav-is closed 1 year ago

dav-is commented 1 year ago

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

baparham commented 1 year ago

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 commented 1 year ago

Ignore pnpm test for node14 like https://github.com/vercel/pkg/pull/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://github.com/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 commented 1 year ago

Commented on the other PR regarding the pnpm issue

emmansun commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

baparham commented 1 year ago

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 commented 1 year ago

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?

emmansun commented 1 year ago

Hi @robertsLando , pls merge this PR, thx!