vega / ts-json-schema-generator

Generate JSON schema from your Typescript sources
MIT License
1.38k stars 190 forks source link

Fix CJS imports #1969

Closed arthurfiorette closed 1 month ago

arthurfiorette commented 1 month ago

Hey guys so unfortunately I think this still doesn't work. Or it's also entirely possible I'm doing something wrong but, whenever I try to import something from ts-json-schema-generator I get this error:

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/stevencummings/workspace/ternary/core-web-server/node_modules/ts-json-schema-generator/cjs/index.js from /Users/stevencummings/workspace/ternary/core-web-server/scripts/docs-gen/Oas3Generator.ts not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead either rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/stevencummings/workspace/ternary/core-web-server/node_modules/ts-json-schema-generator/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

Sorry if this isn't the best place to discuss, lmk if I should open a new issue or what the best thing is to do.

Originally posted by @sacummings91 in https://github.com/vega/ts-json-schema-generator/issues/1964#issuecomment-2130251619

arthurfiorette commented 1 month ago

@domoritz, there's currrently some issues with porting to dual builds:

is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules. literally means that if we want to keep first class ESM support and second class CJS support, we would need to rename all files under /cjs from index.js to index.cjs.

We would also need to make a lot of changes to the exports field inside package.json.

Currently, (until node v22 makes --experimental-require-module stable), CJS code can be imported by ESM and CJS, while ESM code can only be imported by ESM code. What do you think about going back to CJS-only (which fully works in esm projects) instead of making all of these awful changes to support both.

Things were much simpler previously :/

domoritz commented 1 month ago

Changing the exports field isn't that bad I think. We did this in apache arrow: https://cdn.jsdelivr.net/npm/apache-arrow@16.1.0/package.json

domoritz commented 1 month ago

I think the future is ESM so another option is all ESM and anyone who needs CJS uses an old version. Or is there some high-value project that cannot update to ESM?

arthurfiorette commented 1 month ago

Changing the exports field isn't that bad I think

Sure it isn't, but in apache-arrow the type field is declared as commonsjs.

We know that ESM cannot be imported by CJS, but CJS can be imported by both, so is there any benefit to write this library in ESM?

A good landmark in time to port again this library to ESM is when https://nodejs.org/api/cli.html#--experimental-require-module gets stable, so ESM/CJS gets fully interoperable in every way and writing esm/cjs starts being only a matter of taste.

I think the future is ESM

The future is indeed ESM, but until node's gets full support for cjs/esm interop a lot of problems would surface with cjs/esm...

My main usecase with this project is with @kitajs, which uses ts-json-schema-generator from inside a typescript language service plugin that is loaded by the tsserver runtime written in CommonsJS, and as you might imagine, there's the typescript source won't be ported to ESM in the near future.

If this project removes support for CJS, I couldn't use it anymore, just like every other project that might be dependent of a tool written in CJS.

domoritz commented 1 month ago

I see. I personally only need the cli so let's switch back to cjs only until the node ecosystem works better.

navalex commented 1 month ago

Do you have any workaround to make it works for the moment ? Can't make the basic code from the README.md working...

domoritz commented 1 month ago

Use an old version. We will fix this soon.

arthurfiorette commented 1 month ago

Ill be working on this later today