Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/workerd/api/global-scope.c++
Original file line number Diff line number Diff line change
Expand Up @@ -928,8 +928,11 @@ jsg::JsValue ServiceWorkerGlobalScope::getBuffer(jsg::Lock& js) {
// that set the bufferValue, we let's check again.
return p.getHandle(js);
}
auto def = module.get(js, "default"_kj);
auto obj = KJ_ASSERT_NONNULL(def.tryCast<jsg::JsObject>());
// When requireReturnsDefaultExport flag is enabled, resolveModule returns the
// default export directly. Otherwise it returns the module namespace.
auto obj = module.has(js, "default"_kj)
? KJ_ASSERT_NONNULL(module.get(js, "default"_kj).tryCast<jsg::JsObject>())
: module;
auto buffer = obj.get(js, "Buffer"_kj);
JSG_REQUIRE(buffer.isFunction(), TypeError, "Invalid node:buffer implementation");
bufferValue = jsg::JsRef(js, buffer);
Expand Down Expand Up @@ -970,7 +973,9 @@ jsg::JsValue ServiceWorkerGlobalScope::getProcess(jsg::Lock& js) {
// that set the processValue, we let's check again.
return p.getHandle(js);
}
auto def = module.get(js, "default"_kj);
// When requireReturnsDefaultExport flag is enabled, resolveInternalModule returns the
// default export directly. Otherwise it returns the module namespace.
auto def = module.has(js, "default"_kj) ? module.get(js, "default"_kj) : jsg::JsValue(module);
JSG_REQUIRE(def.isObject(), TypeError, "Invalid node:process implementation");
processValue = jsg::JsRef(js, def);
return def;
Expand Down
6 changes: 6 additions & 0 deletions src/workerd/api/node/tests/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,12 @@ wd_test(
data = ["module-create-require-test.js"],
)

wd_test(
src = "module-require-mutable-exports-test.wd-test",
args = ["--experimental"],
data = ["module-require-mutable-exports-test.js"],
)

wd_test(
src = "process-exit-test.wd-test",
args = ["--experimental"],
Expand Down
18 changes: 16 additions & 2 deletions src/workerd/api/node/tests/module-create-require-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,28 @@ export const doTheTest = {
const baz = require('baz');
const qux = require('worker/qux');

strictEqual(foo.default, 1);
// When require_returns_default_export flag is enabled, require() returns the
// default export directly. Otherwise it returns the namespace object.
if (Cloudflare.compatibilityFlags.require_returns_default_export) {
strictEqual(foo, 1);
} else {
strictEqual(foo.default, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
strictEqual(bar, 2);
strictEqual(baz, 3);
strictEqual(qux, '4');

const assert = await import('node:assert');
const required = require('node:assert');
strictEqual(assert, required);

// When require_returns_default_export flag is enabled, require() returns the
// default export directly (assert.default === required).
// When the flag is disabled, require() returns the namespace (assert === required).
if (Cloudflare.compatibilityFlags.require_returns_default_export) {
strictEqual(assert.default, required);
} else {
strictEqual(assert, required);
}

throws(() => require('invalid'), {
message: 'Top-level await in module is not permitted at this time.',
Expand Down
122 changes: 122 additions & 0 deletions src/workerd/api/node/tests/module-require-mutable-exports-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// Copyright (c) 2017-2026 Cloudflare, Inc.
// Licensed under the Apache 2.0 license found in the LICENSE file or at:
// https://opensource.org/licenses/Apache-2.0
import { createRequire } from 'node:module';
import { strictEqual, ok, doesNotThrow } from 'node:assert';

const require = createRequire('/');

export const testTimersPromisesMutable = {
test() {
const timersPromises = require('node:timers/promises');
const originalSetImmediate = timersPromises.setImmediate;
ok(typeof originalSetImmediate === 'function');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const patchedSetImmediate = async function patchedSetImmediate() {
return 'patched';
};

doesNotThrow(() => {
timersPromises.setImmediate = patchedSetImmediate;
});

strictEqual(timersPromises.setImmediate, patchedSetImmediate);
timersPromises.setImmediate = originalSetImmediate;
strictEqual(timersPromises.setImmediate, originalSetImmediate);
},
};

export const testTimersMutable = {
test() {
const timers = require('node:timers');
const originalSetTimeout = timers.setTimeout;
ok(typeof originalSetTimeout === 'function');

const patchedSetTimeout = function patchedSetTimeout() {
return 'patched';
};

doesNotThrow(() => {
timers.setTimeout = patchedSetTimeout;
});

strictEqual(timers.setTimeout, patchedSetTimeout);
timers.setTimeout = originalSetTimeout;
},
};

export const testBufferMutable = {
test() {
const buffer = require('node:buffer');
const originalBuffer = buffer.Buffer;
ok(typeof originalBuffer === 'function');

const patchedBuffer = function PatchedBuffer() {
return 'patched';
};

doesNotThrow(() => {
buffer.Buffer = patchedBuffer;
});

strictEqual(buffer.Buffer, patchedBuffer);
buffer.Buffer = originalBuffer;
},
};

export const testUtilMutable = {
test() {
const util = require('node:util');
const originalPromisify = util.promisify;
ok(typeof originalPromisify === 'function');

const patchedPromisify = function patchedPromisify() {
return 'patched';
};

doesNotThrow(() => {
util.promisify = patchedPromisify;
});

strictEqual(util.promisify, patchedPromisify);
util.promisify = originalPromisify;
},
};

export const testRequireCachesMutableObject = {
test() {
const timersPromises1 = require('node:timers/promises');
const timersPromises2 = require('node:timers/promises');

strictEqual(timersPromises1, timersPromises2);

const patchedSetImmediate = async function patched() {
return 'patched';
};
const original = timersPromises1.setImmediate;

timersPromises1.setImmediate = patchedSetImmediate;
strictEqual(timersPromises2.setImmediate, patchedSetImmediate);
timersPromises1.setImmediate = original;
},
};

// When require_returns_default_export is enabled, require() should return the
// default export directly (which is the object with all the functions),
// not the namespace wrapper with both `default` and named exports.
export const testRequireReturnsDefaultExport = {
test() {
const timers = require('node:timers');
// With require_returns_default_export enabled, timers should be the
// default export object directly, not the namespace wrapper.
// The default export IS the object with setTimeout, setInterval, etc.
ok(typeof timers.setTimeout === 'function');
ok(typeof timers.setInterval === 'function');
ok(typeof timers.clearTimeout === 'function');
ok(typeof timers.clearInterval === 'function');
// The namespace wrapper would have a 'default' property, but when
// we return the default export directly, there's no 'default' property
// on the returned object (unless the default export itself has one).
strictEqual(timers.default, undefined);
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "module-require-mutable-exports-test",
worker = (
modules = [
(name = "worker", esModule = embed "module-require-mutable-exports-test.js")
],
compatibilityFlags = ["nodejs_compat", "require_returns_default_export"],
)
),
],
);
10 changes: 10 additions & 0 deletions src/workerd/io/compatibility-date.capnp
Original file line number Diff line number Diff line change
Expand Up @@ -1316,4 +1316,14 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef {
# Node.js-compatible versions from node:timers. setTimeout and setInterval return
# Timeout objects with methods like refresh(), ref(), unref(), and hasRef().
# This flag requires nodejs_compat or nodejs_compat_v2 to be enabled.

requireReturnsDefaultExport @154 :Bool
$compatEnableFlag("require_returns_default_export")
$compatDisableFlag("require_returns_namespace")
$experimental;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# When enabled, require() will return the default export of a module if it exists.
# If the default export does not exist, it falls back to returning the mutable
# module namespace object. This matches the behavior that Node.js uses for
# require(esm) where the default export is returned when available.
# This flag is useful for frameworks like Next.js that expect to patch module exports.
}
3 changes: 3 additions & 0 deletions src/workerd/io/worker.c++
Original file line number Diff line number Diff line change
Expand Up @@ -1107,6 +1107,9 @@ Worker::Isolate::Isolate(kj::Own<Api> apiParam,
if (features.getEnableNodeJsProcessV2()) {
lock->setNodeJsProcessV2Enabled();
}
if (features.getRequireReturnsDefaultExport()) {
lock->setRequireReturnsDefaultExportEnabled();
}
if (features.getThrowOnUnrecognizedImportAssertion()) {
lock->setThrowOnUnrecognizedImportAssertion();
}
Expand Down
4 changes: 4 additions & 0 deletions src/workerd/jsg/jsg.c++
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ void Lock::setNodeJsProcessV2Enabled() {
IsolateBase::from(v8Isolate).setNodeJsProcessV2Enabled({}, true);
}

void Lock::setRequireReturnsDefaultExportEnabled() {
IsolateBase::from(v8Isolate).setRequireReturnsDefaultExportEnabled({}, true);
}

void Lock::setThrowOnUnrecognizedImportAssertion() {
IsolateBase::from(v8Isolate).setThrowOnUnrecognizedImportAssertion();
}
Expand Down
1 change: 1 addition & 0 deletions src/workerd/jsg/jsg.h
Original file line number Diff line number Diff line change
Expand Up @@ -2647,6 +2647,7 @@ class Lock {

void setNodeJsCompatEnabled();
void setNodeJsProcessV2Enabled();
void setRequireReturnsDefaultExportEnabled();
void setThrowOnUnrecognizedImportAssertion();
bool getThrowOnUnrecognizedImportAssertion() const;
void setToStringTag();
Expand Down
5 changes: 5 additions & 0 deletions src/workerd/jsg/jsvalue.h
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,11 @@ class JsObject final: public JsBase<v8::Object, JsObject> {

int hashCode() const;

// Returns true if this object is an ES module namespace object.
bool isModuleNamespaceObject() const {
return inner->IsModuleNamespaceObject();
}

kj::String getConstructorName() KJ_WARN_UNUSED_RESULT;
JsArray getPropertyNames(Lock& js,
KeyCollectionFilter keyFilter,
Expand Down
9 changes: 7 additions & 2 deletions src/workerd/jsg/modules-new.c++
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,6 @@ class IsolateModuleRegistry final {
// like the CommonJS require. Returns the instantiated/evaluated module namespace.
// If an empty v8::MaybeLocal is returned and the default option is given, then an
// exception has been scheduled.
// Note that this returns the module namespace object. In CommonJS, the require()
// function will actually return the default export from the module namespace object.
v8::MaybeLocal<v8::Object> require(
Lock& js, const ResolveContext& context, RequireOption option = RequireOption::DEFAULT) {
static constexpr auto evaluate = [](Lock& js, Entry& entry, const Url& id,
Expand Down Expand Up @@ -1600,6 +1598,13 @@ JsValue ModuleRegistry::resolve(Lock& js,
ResolveContext::Source source,
kj::Maybe<const Url&> maybeReferrer) {
KJ_IF_SOME(ns, tryResolveModuleNamespace(js, specifier, type, source, maybeReferrer)) {
// When require_returns_default_export flag is enabled and the caller wants
// the "default" export, return the default export directly instead of extracting
// it from the namespace. This matches Node.js require() behavior.
// See: https://github.com/cloudflare/workerd/issues/5844
if (isRequireReturnsDefaultExportEnabled(js) && exportName == "default"_kj) {
return ns.get(js, "default"_kj);
}
return ns.get(js, exportName);
}
JSG_FAIL_REQUIRE(Error, kj::str("Module not found: ", specifier));
Expand Down
41 changes: 40 additions & 1 deletion src/workerd/jsg/modules.c++
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "jsg.h"
#include "setup.h"
#include "util.h"

#include <v8-wasm.h>

Expand Down Expand Up @@ -527,7 +528,45 @@ JsValue ModuleRegistry::requireImpl(Lock& js, ModuleInfo& info, RequireImplOptio
js.v8Context(), v8StrIntern(js.v8Isolate, "default"))));
}

return JsValue(module->GetModuleNamespace());
// When the flag is disabled, return the original module namespace
// to maintain backward compatibility (same object as ESM import returns).
if (!isRequireReturnsDefaultExportEnabled(js)) {
return JsValue(module->GetModuleNamespace());
}

// When require_returns_default_export flag is enabled:
// 1. If module has default export: return it (or a mutable copy if it's a namespace)
// 2. If no default export: return a mutable copy of the namespace
// This matches Node.js require(esm) behavior and allows monkey-patching.
// See: https://github.com/cloudflare/workerd/issues/5844
JsObject moduleNamespace(module->GetModuleNamespace().As<v8::Object>());
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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a PR here: #5869

// import * as _default from 'other-module';
// export default _default;
KJ_IF_SOME(defaultObj, defaultValue.tryCast<JsObject>()) {
if (defaultObj.isModuleNamespaceObject()) {
KJ_IF_SOME(cached, info.maybeMutableExports) {
return JsValue(cached.getHandle(js));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

auto mutableDefault = createMutableModuleExports(js, defaultObj);
info.maybeMutableExports = V8Ref<v8::Object>(js.v8Isolate, mutableDefault);
return mutableDefault;
}
}
// Default export is a regular object (mutable), return it directly
return defaultValue;
}

// No default export - return a mutable copy of the namespace.
KJ_IF_SOME(cached, info.maybeMutableExports) {
return JsValue(cached.getHandle(js));
}
auto mutableExports = createMutableModuleExports(js, moduleNamespace);
info.maybeMutableExports = V8Ref<v8::Object>(js.v8Isolate, mutableExports);
return mutableExports;
}

} // namespace workerd::jsg
6 changes: 6 additions & 0 deletions src/workerd/jsg/modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ class ModuleRegistry {
// For source phase imports - stores the module source object (e.g., WebAssembly.Module)
kj::Maybe<V8Ref<v8::Object>> maybeModuleSourceObject;

// Cache for mutable module exports wrapper when require_returns_default_export flag is enabled.
// Used to ensure require() returns the same mutable object for the same module.
// This enables frameworks like Next.js to patch built-in module exports.
// See: https://github.com/cloudflare/workerd/issues/5844
mutable kj::Maybe<V8Ref<v8::Object>> maybeMutableExports;

ModuleInfo(jsg::Lock& js,
v8::Local<v8::Module> module,
kj::Maybe<SyntheticModuleInfo> maybeSynthetic = kj::none);
Expand Down
9 changes: 9 additions & 0 deletions src/workerd/jsg/setup.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ class IsolateBase {
nodeJsProcessV2Enabled = enabled;
}

inline void setRequireReturnsDefaultExportEnabled(kj::Badge<Lock>, bool enabled) {
requireReturnsDefaultExportEnabled = enabled;
}

inline bool areWarningsLogged() const {
return maybeLogger != kj::none;
}
Expand All @@ -170,6 +174,10 @@ class IsolateBase {
return nodeJsProcessV2Enabled;
}

inline bool isRequireReturnsDefaultExportEnabled() const {
return requireReturnsDefaultExportEnabled;
}

inline bool shouldSetToStringTag() const {
return setToStringTag;
}
Expand Down Expand Up @@ -339,6 +347,7 @@ class IsolateBase {
bool asyncContextTrackingEnabled = false;
bool nodeJsCompatEnabled = false;
bool nodeJsProcessV2Enabled = false;
bool requireReturnsDefaultExportEnabled = false;
bool setToStringTag = false;
bool shouldSetImmutablePrototypeFlag = false;
bool allowTopLevelAwait = true;
Expand Down
Loading
Loading