-
-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESM Support #120
ESM Support #120
Conversation
CI seems not to be happy. I'm ok to bump the major, drop Node v14 and v16 support, and drop all typebox < 0.32, if it simplifies maintenance. |
Hi @mcollina !
Unfortunately, this issue relates specifically to the way TypeScript tries to resolve type definitions for various end user tsconfig.json configurations. Dropping support for older versions of Node may not be necessary, although dropping support for CJS and going full ESM would certainly simplify things a lot (but I don't consider this a feasible option today, but I don't know) In terms of maintenance overhead, you're quite right, however I'm yet to come across good tooling in support of dual build + publishing (I had to roll custom build tooling in TypeBox to support both CJS + ESM). Unfortunately, due to the lack of tooling in this area, it does push back somewhat on libraries to mash builds for both formats. At this point in time, I don't have a good tooling suggestion help simply things, but perhaps the Fastify TS team may have some ideas on community tooling?
I'll take another look at this Btw, I was wondering if you have any general thoughts on dual publishing CJS and ESM? I've been very mindful to retain CJS support in TypeBox, but the preference would be to just move to ESM. I do consider the dual publishing in TypeBox to be a temporary solution in order to provide an avenue to transition to ESM one day, but given the complexity and myriad of other tooling that goes up around most JavaScript projects (as well as downstream integration issues such as this one), I do wonder how other projects are approaching this. Would be super appreciative of any insights you could offer on this aspect of Node development! |
I generically recommend to keep publishing as cjs for libs, mostly because consuming cjs has no problems on Node.js. Everybody recommended to use |
Thanks for the reference to
Mmmm, trying to support ESM has not been without challenges. It's been added to TB to enable bundlers to tree shake the type system, but still mindful of if supporting it is going to cause undue problems downstream (it's like balancing between bundling optimizations and module resolution issues, it's not been fun) If you do notice users reporting TB inference issues on Fastify, please don't hesitate to ping me on those. I'm open to disabling the dual publishing and pursuing other alternatives if the introduction of ESM causes too much confusion for users, but at this stage, still just trying to gauge things. Will review things in January and will likely make a call then. Hope you have a good New Years! :) |
I was merely suggesting ;). This problem is a huge mess. |
Have a good new year as well! |
Checklist
npm run test
andnpm run benchmark
and the Code of conduct
Hi,
This PR adds support for CJS and ESM builds of this Provider. This update was prompted by some users reporting inference issues for TypeBox 0.32.0 which has recently added support for both CJS and ESM.
Issue Overview
Inference issues have been reported by users who configure
tsconfig.json
specifically forThis configuration will have TypeScript resolve the TypeBox ESM definitions when users import
@sinclair/typebox
directly (which is expected). However this Provider internally resolves the TypeBox CJS definitions due to the package being built and configured primarily for CJS.It's been noted that TypeScript will view the ESM definitions imported from
@sinclair/typebox
as incompatible with the CJS definitions internally used by the Provider. What this means is users who upgrade to TB 0.32.0 (and whom have projects that mix and match ESM Types with CJS Types) will almost certainly run into inference issues. Unfortunately, the only user level workaround I know would be to suggest users downgrade configuration toCommonJS
+Node10
resolution, however this configuration may not be feasible in many cases.Additional Information on the Issue can be found here
sinclairzx81/typebox#694 (comment)
To resolve the above, this PR adds an additional build target for ESM, and updates the package.json to correctly resolve either CJS or ESM types irrespective of the users tsconfig.json configuration.
Updates
@arethetypeswrong/cli
(attw)build:clean
- cleans the dist directorybuild:esm
- builds the esm target to dist/esm/index.mjsbuild:cjs
- builds the cjs target to dist/cjs/index.mjs (note extension)build:post
- renames cjs .mjs extension to .js viapost-build.js
build:test
- runs module resolver tests for the packageBefore
The following is the resolution targets before this PR (note CJS for Node 16 (ESM))
Online Link Here
After
The following is the resolution targets after this PR
Submitting for Review