Override module type for CommonJS runtime

[shulhi]

The @rescript/runtime package has "type": "module" in its root package.json, which tells Node.js to treat all .js files as ES modules. However, the files in lib/js/* are actually written in CommonJS format (using require() and exports).

When Node.js tries to load these files via require(), it throws:

  Error [ERR_REQUIRE_ESM]: require() of ES Module ... not supported

This breaks any code that runs directly with Node.js (outside of build tools) because the nearest root package.json has its type set to module.

This follows the https://nodejs.org/api/packages.html#dual-commonjses-module-packages. The nested package.json files override the parent's "type" declaration. This adds explicit package.json file with type set to commonjs (similar to what we have in lib/es6/package.json but with module set).

[cknitt]

Good catch!

Previously, this used to be the other way around. The whole package was common JS, and it was overridden for ES6. That override is still there in https://github.com/rescript-lang/rescript/blob/master/packages/%40rescript/runtime/lib/es6/package.json and could be removed now.

[pkg-pr-new]

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8194

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8194

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8194

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8194

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8194

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8194

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8194

commit: 25d2315

[cknitt]

I wonder why this problem has never surfaced before for v12.

It seems we did not have any kind of test for this after moving tests/tests to esmodule.
Could you add some test setup for this? Maybe in a new folder tests/commonjs_tests?

For the CHANGELOG, could you adapt the wording so that it is clearer what the bug is that is being fixed here?

[cknitt]

I am also asking myself if it would be hard to just explicitly use the corresponding file extensions (.mjs, .cjs) instead. (But not in this PR, here we should keep the fix minimal for easy backportiing. We can investigate this later.)

[shulhi]

@cknitt can you look at the test implementation whether it is sufficient? I tested the test with/without the fix and it caught the error appropriately.

[cknitt]

@cknitt can you look at the test implementation whether it is sufficient? I tested the test with/without the fix and it caught the error appropriately.

@shulhi Looks great, thanks a lot! 👍