lookup: add next

[styfle]

Checklist

• npm test passes
• contribution guidelines followed
  here

Related

• Closes Support pnpm; add Next.js #1017

Co-authored-by: Ethan Arrowood [email protected]

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (419202a) 96.33% compared to head (a693d85) 95.04%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
- Coverage   96.33%   95.04%   -1.29%     
==========================================
  Files          28       28              
  Lines        2181     2181              
==========================================
- Hits         2101     2073      -28     
- Misses         80      108      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[GeoffreyBooth]

Looks good assuming you can get CI to pass. That might be something that needs fixing on the Next side, though, if some of the tests are flaky; maybe a different test command can be created that’s essentially “run the tests that we think should always pass.”

[styfle]

@GeoffreyBooth It looks like windows has been failing on main for awhile https://github.com/nodejs/citgm/commits/main

Furthermore, I'm not sure if citgm actually runs any tests against the lookup.json file that I modified.

[Ethan-Arrowood]

@GeoffreyBooth does the CI run citgm-all ? I think that's the only way lookup.json is used, right?

[GeoffreyBooth]

I’m not very knowledgeable of how CITGM works. Perhaps @targos knows?

[targos]

Here's a first test run: https://github.com/nodejs/citgm/actions/runs/7466807218

[targos]

CI doesn't run citgm-all. It takes too long and must be run manually.

[Ethan-Arrowood]

Looks like it didn't install correctly: https://github.com/nodejs/citgm/actions/runs/7466807218/job/20319001003#step:5:12

[styfle]

This should fix it: vercel/next.js#60443

[styfle]

@targos Can you try once more now that its fixed?

[targos]

https://github.com/nodejs/citgm/actions/runs/7466807218

[Ethan-Arrowood]

Looks like we may need to skip on windows.

Otherwise it seems to have passed on mac and linux 🎉

[styfle]

@targos I added skip win32. Please run once more (Third time's the charm ☘️ )

[targos]

OK, let's do a full run in Jenkins.

[targos]

v18: https://ci.nodejs.org/job/citgm-smoker-nobuild/1579/
v20: https://ci.nodejs.org/job/citgm-smoker-nobuild/1580/
v21: https://ci.nodejs.org/job/citgm-smoker-nobuild/1581/

[styfle]

@targos Looks like pnpm support isn't quite working. Its not selecting the correct version of pnpm

https://ci.nodejs.org/job/citgm-smoker-nobuild/nodes=rhel8-x64/1580/testReport/(root)/citgm/next_v14_0_4/

ERR_PNPM_UNSUPPORTED_ENGINE Unsupported environment (bad pnpm and/or Node.js version)
 Your pnpm version is incompatible with "/home/iojs/tmp/citgm_tmp/12755dbb-db09-4e8a-975e-85e8c56c01ad/next".
 Expected version: 8.14.0
 Got: 8.14.1

I think we need to make sure corepack enable pnpm is run as the first step so the correct version is selected from package.json https://nodejs.org/api/corepack.html

[targos]

pnpm is not installed on CI hosts. It's a dependency of the citgm package: 46d35ae

[styfle]

@targos Interesting. Then perhaps citgm should utilize corepack. Or at least manually read the packageManger field in package.json to select the correct version of the package manger.

[GeoffreyBooth]

@targos Interesting. Then perhaps citgm should utilize corepack. Or at least manually read the packageManger field in package.json to select the correct version of the package manger.

Why does it need to use a precise version of pnpm? Surely 8.14.0 and 8.14.1 aren’t that different?

[styfle]

@GeoffreyBooth Next.js is a highly visible project that receives many PRs. We lock down the package manager version so that contributors fail fast when they have the wrong pnpm installed rather than then submitting an issue saying it doesn't work and we have to ask for system details, etc.

In theory, a patch version should't matter, but in practice it can and there is no reason to leave it open for interpretation. Thats probably the same reason why the packageManager field doesn't allow ranges - it has to be exact.

Thankfully, there is a tool to solve this now: corepack. So most of the citgm logic for package manager selection could probably be deferred to corepack.

[GeoffreyBooth]

Okay, do you want to update CITGM accordingly?

[Ethan-Arrowood]

Would that be this workflow file?

citgm/.github/workflows/test-module.yml

Line 59 in 44548b7

run: npm install

Where is the one that Node.js uses?

[styfle]

I can’t seem to find the place where package managers are installed? Can you share the line of code?

[GeoffreyBooth]

They're just dependencies of CITGM, so they'd get installed via that.

[styfle]

Should I add corepack support to citgm?

Perhaps right before install here?

citgm/lib/package-manager/install.js

Line 99 in 12e6902

const proc = spawn(packageManagerBin, args, options);