Fix require() to return mutable exports for Node.js compat #5847

anonrig · 2026-01-07T19:18:29Z

Fixes #5844 (with the help of our friend Claude)

ES module namespaces have read-only properties by specification, but CommonJS require() is expected to return objects with mutable properties.

jasnell

This is likely breaking and I'm not yet convinced this is the correct fix. Specifically, we have the EXPORT_DEFAULT option and corresponding compat flag that is supposed to fix the issue. The problem is that the original implementation of require() returned the module namespace and not the default export. The compat flag is supposed to update that so that it returns the default export, which is the correct behavior. When that compat flag is not set, it's supposed to maintain the original behavior of returning the module namespace. Before applying any other fixes like this here, we need to investigate if/why the EXPORT_DEFAULT option is not being correctly set.

anonrig · 2026-01-07T19:43:37Z

@jasnell I investigated and the EXPORT_DEFAULT mechanism is working correctly - the issue is much more in depth than it initially appeared.

EXPORT_DEFAULT is only set for synthetic modules (not for ESM modules)
Node built-in modules are ESM modules, not synthetic. They're compiled from TS to JS, so maybeSynthetic == kj::none and EXPORT_DEFAULT is intentionally not set for them.
Even if we set EXPORT_DEFAULT for node:timers/promises, the default export of that module will be the namespace object (which is read-only). So we'd still get the same exact error.

I don't see any other way of solving this than the current approach.

jasnell · 2026-01-07T19:51:21Z

As mentioned, the proposed approach is likely breaking. This will require more consideration on what the correct approach is. I'm working on streams stuff at the moment but will be able to come back to this either later today or tomorrow.

jasnell · 2026-01-08T21:32:20Z

Ok, yeah I remember now about the limitations on the existing compat flag. What needs to happen then is introducing another compat flag that essentially says: for any module type, when require() is used, return the default export if it exists, and if it does not, fallback to the behavior you have here. However, if this new compat flag is turned off, the current behavior should be preserved. The general idea is that require should always return the default export, regardless of the module type.

vicb · 2026-01-09T16:45:42Z

src/workerd/api/node/tests/module-create-require-test.js

+    if (Cloudflare.compatibilityFlags.require_returns_default_export) {
+      strictEqual(foo, 1);
+    } else {
+      strictEqual(foo.default, 1);


/cc @petebacondarwin FYI (wondering if Devin did not write code relying on that)

vicb

When #5844 uses require, that's before the ESBuild bundling phase.

So there should be import in the end.

Note that the issue has import based code working in Node and failing in workerd.

So I think that's something we make sure works and should test.

I won't have time to check tonight but can probably find time to check by Monday.

Another option is to merge that but as experimental until we can double check it actually cover all the use cases and only remove exp / set a default date then.

src/workerd/io/compatibility-date.capnp

vicb

I added a repro in #5844 (comment)

This PR does not fix it

jasnell · 2026-01-12T18:42:42Z

src/workerd/jsg/modules.c++

+  if (moduleNamespace.has(js, "default"_kj)) {
+    auto defaultValue = moduleNamespace.get(js, "default"_kj);
+    // If the default export is itself a module namespace object (read-only),
+    // we need to create a mutable copy. This happens when a module does:


Hmm... is this actually correct tho? Why wouldn't we just recursively grab the default off the returned module namespace and keep going until we either get a non namespace value or nothing. If we do get nothing, then we can fall back to the mutable wrapper.

It looks like this is meant to account for cases like in our impl of src/node/timers/promises.ts re-exporting the internal module namespace. I think this approach is a mistake. Instead, we should export a proper default object.

That is, change the impl in timers/promises.ts to something like...

import { setTimeout, setImmediate, setInterval, scheduler, } from 'node-internal:internal_timers_promises'; export * from 'node-internal:internal_timers_promises'; export default { setTimeout, setImmediate, setInterval, scheduler, };

If we go with this approach, whats the best way to avoid regressions?

Actually, it would need to be something like..

import { setTimeout, setImmediate, setInterval, scheduler, } from 'node-internal:internal_timers_promises'; export { setTimeout, setImmediate, setInterval, scheduler, }; export default { setTimeout, setImmediate, setInterval, scheduler, } as typeof import('node:timers/promises');

If we go with this approach, whats the best way to avoid regressions?

Regressions like what?

To make sure we don't return an imported attribute as a export default statement (just like the node:timers/promises one...)

You mean regressing back to exporting the namespace as the default? We could likely craft an eslint rule for that... but even more basic would just be adding appropriate new tests.

I've opened a PR here: #5869

jasnell · 2026-01-12T18:46:39Z

src/workerd/jsg/modules.c++

+      if (defaultObj.isModuleNamespaceObject()) {
+        KJ_IF_SOME(cached, info.maybeMutableExports) {
+          return JsValue(cached.getHandle(js));
+        }


you could like simplify a bit by moving the check for the cached mutableExports out of this block and immediately after the compat flag check. The cached value would only have been set if require already happened.

jasnell · 2026-01-12T18:49:13Z

src/workerd/io/compatibility-date.capnp

+  requireReturnsDefaultExport @154 :Bool
+    $compatEnableFlag("require_returns_default_export")
+    $compatDisableFlag("require_returns_namespace")
+    $experimental;


Nit... once this is not experimental, it should likely be implied by the original compat flag for these cases (exportCommonJsDefaultNamespace)... or have that one implied by this one.

jasnell · 2026-01-12T18:53:34Z

src/workerd/api/node/tests/module-require-mutable-exports-test.js

+  test() {
+    const timersPromises = require('node:timers/promises');
+    const originalSetImmediate = timersPromises.setImmediate;
+    ok(typeof originalSetImmediate === 'function');


Recommend expanding the test a bit to demonstrate that...

const timersPromises = require('node:timers/promises'); const { default: timersPromises2 } = await import('node:timers/promises'); assert.strictEqual(timersPromises, timersPromises2); // but that... const timersPromises3 = await import('node:timers/promises'); assert.notStrictEqual(timersPromises, timersPromises3);

Also, overriding the method on the default export or mutable wrapper should have no impact on original named export on the module namespace. That should be tested also.

jasnell · 2026-01-12T18:54:31Z

src/workerd/api/node/tests/module-import-mutable-test.js

+
+export const testTimersPromisesMutable = {
+  async test() {
+    const timersPromises = await import('node:timers/promises');


Suggested change

const timersPromises = await import('node:timers/promises');

const { default: timersPromises } = await import('node:timers/promises');

anonrig requested a review from jasnell January 7, 2026 19:18

anonrig requested review from a team as code owners January 7, 2026 19:18

jasnell requested changes Jan 7, 2026

View reviewed changes

This comment was marked as off-topic.

Sign in to view

vicb mentioned this pull request Jan 9, 2026

[BUG] Next.js 16.1.0 crashes on Cloudflare Workers (Error 1101): Cannot assign to read only property 'setImmediate' of [object Module] opennextjs/opennextjs-cloudflare#1049

Open

anonrig force-pushed the yagiz/make-modules-mutable branch 6 times, most recently from a74d0bf to 028bd7b Compare January 9, 2026 16:07

anonrig requested a review from jasnell January 9, 2026 16:07

anonrig force-pushed the yagiz/make-modules-mutable branch 2 times, most recently from 6dc810f to 41a9458 Compare January 9, 2026 16:11

Fix require() to return mutable exports for Node.js compat

f7c4cd0

anonrig force-pushed the yagiz/make-modules-mutable branch from 41a9458 to f7c4cd0 Compare January 9, 2026 16:15

vicb reviewed Jan 9, 2026

View reviewed changes

danlapid reviewed Jan 9, 2026

View reviewed changes

src/workerd/io/compatibility-date.capnp Outdated Show resolved Hide resolved

anonrig commented Jan 9, 2026

View reviewed changes

src/workerd/io/compatibility-date.capnp Outdated Show resolved Hide resolved

Update compatibility-date.capnp

26ba854

vicb requested changes Jan 12, 2026

View reviewed changes

test mutable import

e4ba6a3

jasnell reviewed Jan 12, 2026

View reviewed changes

fix test

7903a1b

	const timersPromises = await import('node:timers/promises');
	const { default: timersPromises } = await import('node:timers/promises');

Fix require() to return mutable exports for Node.js compat #5847

Are you sure you want to change the base?

Fix require() to return mutable exports for Node.js compat #5847

Uh oh!

Conversation

anonrig commented Jan 7, 2026

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

anonrig commented Jan 7, 2026

Uh oh!

jasnell commented Jan 7, 2026

Uh oh!

jasnell commented Jan 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants