chore(treewide): natively support CommonJS #334
Conversation
🦋 Changeset detectedLatest commit: 976821c The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -1,12 +1,12 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "module": "ES2020", | |||
| "module": "ES2015", | |||
There was a problem hiding this comment.
what's the oldest node version we want to support?
There was a problem hiding this comment.
unsure but if it works with es2015 it'll work with all of them. before es2015 there was no const/let, so it's not worth going back any more
| "build:tsc": "pnpm prebuild && tsc && cp -r src/napi dist/", | ||
| "build": "pnpm build:tsc && napi build --platform --release --dts native.d.ts --js native.cjs --pipe \"prettier -w\" src/napi", | ||
| "artifacts": "pnpm build:tsc && napi artifacts", | ||
| "build:ts": "pnpm prebuild && tsup --onSuccess \"tsc --declaration --emitDeclarationOnly\" && cp -r src/napi dist/ && cp -r src/napi/* dist/", |
There was a problem hiding this comment.
this line is silly but it's to get around esbuild's issues with dynamic import of fs
There was a problem hiding this comment.
is the order of things correct here? it looks like it first copies napi files but napi build is only called afterwards in the build script 👀
There was a problem hiding this comment.
hm it was like this also before, just not sure I understand the reason 🤷
There was a problem hiding this comment.
there's a few files in src/napi already pre-generation, namely the shim to translate them from CJS to ESM if needed. i do think this might be a bit more convoluted than necessary but it works and doesn't seem to take too long on this part
| "build": "pnpm build:tsc && napi build --platform --release --dts native.d.ts --js native.cjs --pipe \"prettier -w\" src/napi", | ||
| "artifacts": "pnpm build:tsc && napi artifacts", | ||
| "build:ts": "pnpm prebuild && tsup --onSuccess \"tsc --declaration --emitDeclarationOnly\" && cp -r src/napi dist/ && cp -r src/napi/* dist/", | ||
| "build": "pnpm build:ts && napi build --platform --release --dts native.d.ts --js native.cjs --pipe \"prettier -w\" src/napi", |
There was a problem hiding this comment.
this looks like napi is only building for cjs, is that the behaviour we want?
There was a problem hiding this comment.
we have a wrapper for esm, napi only generates CJS
There was a problem hiding this comment.
update: this doesn't actually work, tried using agents and it broke. unsure where to go from here because napi doesn't do ESM
There was a problem hiding this comment.
what's the error you're running into?
There was a problem hiding this comment.
something about not being able to find the native.cjs file when running it under cjs. can't reproduce right now, i'm having a different issue related to tsup on Agents in specific
There was a problem hiding this comment.
upon further testing with node-sdks examples directly this doesn't appear to be reproducible 🤷♀️
nbsp
changed the title
closes #304