-
Notifications
You must be signed in to change notification settings - Fork 510
Fix require() to return mutable exports for Node.js compat #5847
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,25 @@ | ||||||
| // 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 { strictEqual, ok, doesNotThrow } from 'node:assert'; | ||||||
|
|
||||||
| export const testTimersPromisesMutable = { | ||||||
| async test() { | ||||||
| const timersPromises = await import('node:timers/promises'); | ||||||
|
||||||
| const timersPromises = await import('node:timers/promises'); | |
| const { default: timersPromises } = await import('node:timers/promises'); |
| 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-import-mutable-test", | ||
| worker = ( | ||
| modules = [ | ||
| (name = "worker", esModule = embed "module-import-mutable-test.js") | ||
| ], | ||
| compatibilityFlags = ["nodejs_compat", "require_returns_default_export"], | ||
| ) | ||
| ), | ||
| ], | ||
| ); |
| 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'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recommend expanding the test a bit to demonstrate that... 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"], | ||
| ) | ||
| ), | ||
| ], | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| #include "jsg.h" | ||
| #include "setup.h" | ||
| #include "util.h" | ||
|
|
||
| #include <v8-wasm.h> | ||
|
|
||
|
|
@@ -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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 That is, change the impl in import {
setTimeout,
setImmediate,
setInterval,
scheduler,
} from 'node-internal:internal_timers_promises';
export * from 'node-internal:internal_timers_promises';
export default {
setTimeout,
setImmediate,
setInterval,
scheduler,
};
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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');
Regressions like what?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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...)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment.
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)