Migrate to ESM #484
Migrate to ESM #484
Conversation
|
| @@ -0,0 +1 @@ | |||
| v22.9.0 | |||
There was a problem hiding this comment.
I am fuzzy on this. This repo only has to support one node version. I had issues using --experimental-strip-types on node 20 so I decided to use node 22 here - for dev workflows and on CI. But then only node 20 is currently supported for GitHub Actions (using: 'node22' doesn't work yet).
| }, | ||
| "scripts": { | ||
| "build": "esbuild src/index.ts --bundle --platform=node --target=node20 --minify --outfile=dist/index.js", | ||
| "build": "esbuild src/index.ts --bundle --platform=node --target=node20 --format=esm --minify --outfile=dist/index.js", |
There was a problem hiding this comment.
In theory... this should be quite safe but then... who knows. Perhaps this should only land after testing this e2e in some repo. It feels like a somewhat risky change.
There was a problem hiding this comment.
I used to migrate to esbuild & esm in my actions but reverted back due to some issues I've forgotten, so probably good to look out here too.
There was a problem hiding this comment.
Damn. That's not great :p I think I'll still try to run with it - but I have to definitely test it e2e first
src/run.ts
Outdated
| fileURLToPath( | ||
| resolve( | ||
| "@changesets/cli/bin.js", | ||
| pathToFileURL(path.join(cwd, "x.cjs")).toString() | ||
| ) | ||
| ), |
There was a problem hiding this comment.
this is... quite a dance to make it work. Not the biggest issue but I wish there would be a builtin way to make this easier.
If I'm not mistaken, it also means that a bug was introduced here:
https://github.com/changesets/changesets/blob/268a29fedc948f22c672a3b1e3e51df4427f478d/packages/cli/src/commit/getCommitFunctions.ts#L16
A directory can't be used directly as input to resolve. It should be a filepath. cc @bluwy for double-checking my logic
There was a problem hiding this comment.
Yeah I think you're right. That should probably be a file path otherwise it resolves from one directory higher, which probably coincidentally worked because there's no node_modules in .changeset/ for the linked code.
About this code, could we also use require.resolve instead of import-meta-resolve too?
There was a problem hiding this comment.
About this code, could we also use require.resolve instead of import-meta-resolve too?
Yep, that was my plan - I just wanted to have a pushed out reference of how to do this using import-meta-resolve ;p I simplified this now.
| import path from "node:path"; | ||
| import pkgJson from "../package.json" with { type: "json" }; | ||
|
|
||
| process.chdir(path.join(import.meta.dirname, "..")); |
There was a problem hiding this comment.
We only use import.meta.dirname in tests and script so far so it's fine anyway. But I wonder if there are any compatibility gotchas to consider if we'd use this in the code of the action itself.
I'm not sure what is the caveat the docs mention here:
Caveat: only present on file: modules.
Is that only that it wouldn't be present for modules imported/downloaded using HTTP(s) etc?
There was a problem hiding this comment.
Yeah I think that's what it means. On GitHub workflows the code should be ran as files though and shouldn't have this issue, though you need to account for it resolving from dist instead.
There was a problem hiding this comment.
though you need to account for it resolving from dist instead.
Good point. It's not an issue right now as we don't use it in src (and all files are nested directly in src too). But I'll keep that in mind as something to be more careful about.
| const requireChangesetsCliPkgJson = (cwd: string) => { | ||
| try { | ||
| return require(resolveFrom(cwd, "@changesets/cli/package.json")); | ||
| return require(require.resolve("@changesets/cli/package.json", { |
There was a problem hiding this comment.
I'm not sure why we were using resolveFrom. require.resolve supported this for a long time already. It also just seems like using require here is an easier option than dancing around this using import-meta-resolve.
* upstream/main: (28 commits) Version Packages (changesets#480) Fixed missed `__dirname` reference (changesets#496) Switch to bundling with Rollup (changesets#495) Migrate to ESM (changesets#484) Fixed situations in which `cwd` was specified as a relative path and used with (default) `commitMode: git-cli` (changesets#486) Add LICENSE file (changesets#491) Fix PRs sometimes not getting reopened with `commitMode: github-api` (changesets#488) Removed `fs-extra` dependency (changesets#481) Setup Git user in `release-pr` workflow (changesets#493) Use proper ndoe version in `release-pr` workflow (changesets#492) Add `release-pr` workflow (changesets#490) Migrate to Vitest (changesets#483) Switch to `esbuild` for bundling (changesets#479) Import only for `semver/functions/lt` (changesets#482) Avoid hitting a deprecation warning when encountering errors from `@octokit/request-error` (changesets#461) Run typecheck on CI (changesets#478) Updated `@actions/*` and `@octokit/*` dependencies (changesets#477) Bump @babel/runtime from 7.21.5 to 7.27.1 (changesets#464) Version Packages (changesets#476) Make git add work consistently with subdirectories (changesets#473) ...
No description provided.