chore: migrate to dual publish #166
Conversation
CatchMe2
requested review from
kibertoad,
drdaemos,
CarlosGamero and
a team
as code owners
| "moduleResolution": "node", | ||
| "esModuleInterop": true, | ||
| "resolveJsonModule": true, | ||
| "noEmit": true, |
There was a problem hiding this comment.
why do we use noEmit by default?
There was a problem hiding this comment.
typescript is used only for typechecking now, building is handled by tsup
| "build": "tsc", | ||
| "build": "tsup", |
There was a problem hiding this comment.
Just out of curiosity, and most likely I lack some knowledge 😓(asking also to learn :D) so please bear with me.
AFAIK the current build system works with cjs and esm services and we are migrating our services to ESM, could make sense to wait until all services are migrated and then just go with ESM?
Sorry if the question is very stupid 🙏
There was a problem hiding this comment.
Previously, we were building to cjs only, with tsup we are doing dual-build. In the dist there are .js (ESM) and .cjs (CJS) now. Besides that, the current approach prevents from accessing subpaths in imports.
Once all services are rewritten to ESM we can get rid of CJS entirely.
There was a problem hiding this comment.
Considering that a lot of stuff depend on node-core, including some external libraries like message-queue-toolkit, which are used outside of just Lokalise, and forcing all of them to go ESM-only wouldn't be particularly nice. I'd say that dual publish makes sense until at least Node 24, which will finally support ESM imports in CJS world.
There was a problem hiding this comment.
ESM doesn't work without dual-publish. Based on local test using commonjs build only
// tmp.mjs
import { removeDuplicates } from '@lokalise/node-core'
console.log(removeDuplicates([1, 1]))
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in node_modules/@lokalise/node-core/package.json imported from polyglot-service/tmp.mjs
There was a problem hiding this comment.
@kibertoad yeah, we shouldn't go ESM only before Node 24 LTS
There was a problem hiding this comment.
I see! thanks for explanation it makes a lot of sense 🙏
Changes
This PR implements dual publish with tsup.
A new "exports" field was added to the package.json that replaces "main"/"types" fields.
tsconfig.json was updated to work as a typecheck process only, and a few strict params were added like verbatimModuleSyntax or "moduleDetection": "force".
It is a major release as it will break the codebase if any import is pointing to the submodules (@lokalise/node-core/dist/xxx)
Checklist
major,minor,patchorskip-release