[build] Roll pnpm dependencies #2749
Conversation
Some updates are not included to avoid breakage (kicking the can down the road), but we make progress on a number of packages.
Node 16 has been EOL since August 2023.
| @@ -117,17 +117,17 @@ jobs: | |||
| - run: mkdir -p npm/workerd/lib | |||
| - run: mkdir -p npm/workerd/bin | |||
| - name: Build node-install | |||
| run: npx esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node16 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning | |||
| run: npx esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node18 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning | |||
There was a problem hiding this comment.
Before this step, we should call pnpm@setup github workflow and install the dependency.
npx esbuild will install the latest version, if it doesn't exist.
| run: npx esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node18 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning | |
| run: node_modules/.bin/esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node18 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning |
There was a problem hiding this comment.
Maybe this is best addressed in another PR aimed at improving the NPM CI job – bumping the node version here makes sense within the scope of updating npm packages, but I've never looked at how we build these files in detail
| env: | ||
| WORKERD_VERSION: ${{ needs.version.outputs.version }} | ||
| LATEST_COMPATIBILITY_DATE: ${{ needs.version.outputs.date }} | ||
| - name: Build node-shim | ||
| run: npx esbuild npm/lib/node-shim.ts --outfile=npm/workerd/bin/workerd --bundle --target=node16 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning | ||
| run: npx esbuild npm/lib/node-shim.ts --outfile=npm/workerd/bin/workerd --bundle --target=node18 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning |
There was a problem hiding this comment.
What's the reason for not targeting LTS? node18 active support ended, and even node 20(LTS) will end in 1 month.
There was a problem hiding this comment.
My thinking was that we should target the oldest version developers would reasonably be using, I'm not sure how quickly folks update to the latest version in the Node.js ecosystem but as long as 18 is being supported we should target since it (presumably) costs us nothing for this small application
There was a problem hiding this comment.
It's hard to distinguish when a technology is not used. Are you thinking about EOL change to drop node18?
There was a problem hiding this comment.
Actually looked at the generated output – it's identical across node versions which is unsurprising since there's not a whole lot of code to begin with. The esbuild version only has cosmetic differences between 0.15 and 0.23, so it is inconsequential too. I think this can go ahead as-is, the node18 label is least likely to cause confusion IMHO.
| @@ -117,17 +117,17 @@ jobs: | |||
| - run: mkdir -p npm/workerd/lib | |||
| - run: mkdir -p npm/workerd/bin | |||
| - name: Build node-install | |||
| run: npx esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node16 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning | |||
| run: npx esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node18 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning | |||
There was a problem hiding this comment.
PS: We are also using pnpm but running eslint using npx.
Some updates are not included to avoid breakage (kicking the can down the road), but we make progress on a number of packages.
Node 16 has been EOL since August 2023.
=======================
This is a general update PR, I've updated dependencies here before but that was usually focused on specific packages. This does not attempt to update Typescript to 5.6 or update typescript-eslint beyond 8.4.0 (causes lint failures that go over my head).
@penalosa: Any objections to updating the NPM deployment target to node18? Would we need to document this anywhere? I'm also apprehensive about updating esbuild since it's quite outdated by now, it could break the NPM job and we'd only find out when trying to release
@anonrig Let me know if you have concerns too. This is intended to be mostly safe. Since none of the files being updated is used by the downstream build I'll make a similar downstream PR afterwards.