search

shakacode / shakapacker

Use Webpack to manage app-like JavaScript modules in Rails
MIT License
400 stars 89 forks source link

Shakapacker 8: usage of corepack, an experimental extension, for a stable release #478

Closed tagliala closed 1 month ago

tagliala commented 1 month ago

Hi,

I'm trying to upgrade to Shakapacker 8 and I ran into this warning:

Presence of the "packageManager" field indicates that the project is meant to be used with Corepack, a tool included by default with all official Node.js distributions starting from 16.9 and 14.19.
Corepack must currently be enabled by running corepack enable in your terminal. For more information, check out https://yarnpkg.com/corepack.

When checking information about Corepack, I'm reading:

The "packageManager" field defines which package manager is expected to be used when working on the current project. It can be set to any of the supported package managers, and will ensure that your teams use the exact same package manager versions without having to install anything else other than Node.js.

This field is currently experimental and needs to be opted-in; check the Corepack page for details about the procedure.

Is it possible to opt out from corepack?

tomdracz commented 1 month ago

packageManager field is used by us to determine which manager to use as we now support plenty of others and not just yarn. See https://github.com/shakacode/shakapacker/blob/main/docs/v8_upgrade.md#the-packagemanager-property-in-packagejson-is-used-to-determine-the-package-manager

This is independent of corepack though and we're not forcing the opt-in to that. Although those warnings might get annoying.

Cc @G-Rath

tomdracz commented 1 month ago

https://github.com/yarnpkg/yarn/issues/9015 actually I do wonder if Yarn is not making this difficult as they seem to not like the presence of the field in package.json but without corepack 😬

@tagliala Is it the warning you are getting or an error that kills some process?

G-Rath commented 1 month ago

my rails-template pr which has been smoke testing this is passing across all package managers and versions, and I'm not seeing that warning anywhere; I'm just trying to check if it's because it is going to stderr and that that is not being outputted.

I also can't actually find any part of that warning in Yarn Berry's code, which is weird...

G-Rath commented 1 month ago

In fact, I'm pretty sure this warning shouldn't be a problem because if you do yarn init -2 -y it generates a package.json with packageManager set regardless of if you use corepack:

workspace/projects-scrap/yarn-check took 3s
❯ yarn init -2 -y
warning The yes flag has been set. This will automatically answer yes to all questions, which may have security implications.
➀ YN0000: You don't seem to have Corepack enabled; we'll have to rely on yarnPath instead
➀ YN0000: Downloading https://repo.yarnpkg.com/4.2.2/packages/yarnpkg-cli/bin/yarn.js
➀ YN0000: Saving the new release in .yarn/releases/yarn-4.2.2.cjs
➀ YN0000: Done with warnings in 0s 629ms
➀ YN0000: · Yarn 4.2.2
➀ YN0000: β”Œ Resolution step
➀ YN0000: β”” Completed
➀ YN0000: β”Œ Fetch step
➀ YN0000: β”” Completed
➀ YN0000: β”Œ Link step
➀ YN0000: β”” Completed
➀ YN0000: · Done in 0s 58ms

yarn-check on ξ‚  main [?] via  v20.11.0
❯ yarn install
na➀ YN0000: · Yarn 4.2.2
➀ YN0000: β”Œ Resolution step
➀ YN0000: β”” Completed
➀ YN0000: β”Œ Fetch step
➀ YN0000: β”” Completed
➀ YN0000: β”Œ Link step
➀ YN0000: β”” Completed
➀ YN0000: · Done in 0s 29ms

yarn-check on ξ‚  main [?] via  v20.11.0
❯ cat package.json
{
  "name": "yarn-check",
  "packageManager": "yarn@4.2.2"
}
G-Rath commented 1 month ago

Yeah I've played around with this in a few different ways without being able to trigger this warning - Yarn v1, v3, and v4, with corepack uninstalled, installed but disabled, and installed + enabled.

Not saying it's impossible, but I think it's either very dependent on versions or I've forgotten about some config or env variable that's very undocumented (but at least then it would mean it can be disabled πŸ˜…)

tagliala commented 1 month ago

Sorry, I'm using classic yarn (as I always did since webpacker was released)

$ yarn --version
1.22.21

I'm not able to replicate this at the moment, but I think that "corepack disable" broke something, since yarn is not available anymore (I had previously run corepack enable to check if it worked)

G-Rath commented 1 month ago

think that "corepack disable" broke something, since yarn is not available anymore (I had previously run corepack enable to check if it worked)

Yeah because of what corepack is doing, it can get a bit weird if you disable it - ultimately you should always be able to npm un -g corepack; npm i -g yarn to definitely get back to a working state (I swear they use to have a section outlining that, but it seems to have gone away)

tagliala commented 1 month ago

Thanks. I've tried but the warning does not appear anymore.

I've also noticed that now it installs 1.22.22 but I've reinstalled 1.22.21 and still no warning

G-Rath commented 1 month ago

Cool, that matches with the results I've gotten after trying a bunch of different combos and versions πŸ˜…

fwiw, I think I've found it: this is a message in Yarn classic which was introduced in v1.22.20, after which it was then updated to check for SKIP_YARN_COREPACK_CHECK (which I don't have set).

The current latest version of the code is https://github.com/yarnpkg/yarn/blob/740c38c3a962c30ddb344a919bbfb7065620714b/src/cli/index.js#L292-L321

I've still not actually been able to trigger it myself but that doesn't surprise me as while digging into this I realised it shouldn't make sense for this to show frequently because the point of the packageManager field is meant to be that you can opt-in, and especially for Yarn v1 (i.e. Yarn v4+ would really like you to use corepack but if they want to make that the cost of using newer versions that's their choice).

The code itself looks like it should only fire if you have packageManager to set not Yarn v1 while using Yarn - which would mean you're doing it wrong anyway (as you should be using the package manager version matching packageManager), and also align further with my previous paragraph.

G-Rath commented 1 month ago

derp ok I've reproduced this, and it is an process-stopping error (which is indicated by the code anyway), but I think it's actually user error - it happens if you set packageManager to a version of Yarn that isn't v1 while using v1.

The full message is important:

❯ yarn install
error This project's package.json defines "packageManager": "yarn@2". However the current global version of Yarn is 1.22.22.

Presence of the "packageManager" field indicates that the project is meant to be used with Corepack, a tool included by default with all official Node.js distributions starting from 16.9 and 14.19.
Corepack must currently be enabled by running corepack enable in your terminal. For more information, check out https://yarnpkg.com/corepack.

So the actual error here is that you are using the wrong package manager / version - regardless of corepack, that is still a valid error

tomdracz commented 1 month ago

Closing based on the above. Full error message makes it relatively clear on what's the issue

tagliala commented 1 month ago

So the actual error here is that you are using the wrong package manager / version - regardless of corepack, that is still a valid error

Thanks, yes. I was able to reproduce:

  "engines": {
    "node": ">= 20.0",
    "yarn": "^1.22.21"
  },
  "packageManager": "yarn@1",
~/.rvm/gems/ruby-3.3.1/gems/package_json-0.1.0/lib/package_json/managers/yarn_classic_like.rb:62:in `fetch_bin_path': yarn bin failed with exit code 1: error This project's package.json defines "packageManager": "yarn@1". However the current global version of Yarn is 1.22.22. (PackageJson::Error)

Presence of the "packageManager" field indicates that the project is meant to be used with Corepack, a tool included by default with all official Node.js distributions starting from 16.9 and 14.19.
Corepack must currently be enabled by running corepack enable in your terminal. For more information, check out https://yarnpkg.com/corepack.
davidwessman commented 1 month ago

On Heroku, with Yarn Berry (4.1.1) I get the following error:

remote:         !     Yarn release script may conflict with "packageManager"
remote:        
remote:               The package.json file indicates the target version of Yarn to install with:
remote:               - "packageManager": "yarn@4.1.1"
remote:        
remote:               But the .yarnrc.yml configuration indicates a vendored release of Yarn should be used with:
remote:               - yarnPath: ".yarn/releases/yarn-4.1.1.cjs"
remote:        
remote:               This will cause the buildpack to install yarn@4.1.1 but, when running Yarn commands, the vendored release
remote:               at ".yarn/releases/yarn-4.1.1.cjs" will be executed instead.
remote:        
remote:               To ensure we install the version of Yarn you want, choose only one of the following actions:
remote:               - Remove the "packageManager" field from package.json
remote:               - Remove the "yarnPath" configuration from .yarnrc.yml and delete the vendored release at ".yarn/releases/yarn-4.1.1.cjs"
remote:               https://devcenter.heroku.com/articles/nodejs-support
remote:

my setup was done based on this issue: https://github.com/heroku/heroku-buildpack-nodejs/issues/1223 where I followed the migration described by Heroku: https://devcenter.heroku.com/articles/migrating-from-yarn-classic

but had to add it using yarn set version berry --yarn-path.

And then when I upgraded to Shakapacker 8 I had to add it to package.json, but now I have a conflict. I think I tried to add it to the package.json for Heroku in the beginning as well, but that was not enough.

I can open in a separate issue if you think this is not related, but I am not 100% sure how to handle this configuration anymore.

G-Rath commented 1 month ago

@davidwessman that's a bug with the Heroku build pack - it's perfectly valid to have both yarnPath and packageManager set (we've got this in eslint-plugin-jest) and in fact iirc Yarn itself sets this; plus the versions themselves match - at best, the buildpack should determine that the versions match and so not warn.

Is this actually failing your builds though? the [pull request that added this] says

If a vendored binary is present and a Yarn version is set in the packageManager field of package.json then the vendored binary will be used and a warning will be displayed noting this.

davidwessman commented 1 month ago

@davidwessman that's a bug with the Heroku build pack - it's perfectly valid to have both yarnPath and packageManager set (we've got this in eslint-plugin-jest) and in fact iirc Yarn itself sets this; plus the versions themselves match - at best, the buildpack should determine that the versions match and so not warn.

Is this actually failing your builds though? the [pull request that added this] says

If a vendored binary is present and a Yarn version is set in the packageManager field of package.json then the vendored binary will be used and a warning will be displayed noting this.

The builds are not failing, this seems to be a working setup except for the warnings about duplicate configuration. There is still an open issue on the heroku buildpack, so lets hope they figure something out that works without warnings as well πŸ™‚