From 8063e9d6b8b4114a436aa7eaa37b5147b8cbe877 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 20 Oct 2024 15:02:44 -0700 Subject: [PATCH] Fixes #14411 (#14691) --- cmake/tools/SetupWebKit.cmake | 2 +- src/bun.js/bindings/CommonJSModuleRecord.cpp | 22 +++++++++++--- src/bun.js/bindings/ZigGlobalObject.cpp | 30 ++++++++++++++++++- src/bun.js/bindings/bindings.cpp | 4 +-- .../modules/AbortControllerModuleModule.h | 2 +- src/js/builtins/BunBuiltinNames.h | 1 - src/js/builtins/Module.ts | 11 +++++++ test/cli/run/esm-defineProperty.test.ts | 11 ++++++- .../bun/resolve/esModule-annotation.test.js | 4 +-- test/js/bun/resolve/esModule.test.ts | 27 +++++++++++++++++ 10 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 test/js/bun/resolve/esModule.test.ts diff --git a/cmake/tools/SetupWebKit.cmake b/cmake/tools/SetupWebKit.cmake index 7c189262f5..5a701ad821 100644 --- a/cmake/tools/SetupWebKit.cmake +++ b/cmake/tools/SetupWebKit.cmake @@ -2,7 +2,7 @@ option(WEBKIT_VERSION "The version of WebKit to use") option(WEBKIT_LOCAL "If a local version of WebKit should be used instead of downloading") if(NOT WEBKIT_VERSION) - set(WEBKIT_VERSION 12e2f46fb01f7c5cf5a992b9414ddfaab32b7110) + set(WEBKIT_VERSION 543cca2140eafdba845f6689024abaac0d9924f5) endif() if(WEBKIT_LOCAL) diff --git a/src/bun.js/bindings/CommonJSModuleRecord.cpp b/src/bun.js/bindings/CommonJSModuleRecord.cpp index baf862139f..ac0f28d6f3 100644 --- a/src/bun.js/bindings/CommonJSModuleRecord.cpp +++ b/src/bun.js/bindings/CommonJSModuleRecord.cpp @@ -763,7 +763,7 @@ void populateESMExports( bool ignoreESModuleAnnotation) { auto& vm = globalObject->vm(); - const Identifier& esModuleMarker = builtinNames(vm).__esModulePublicName(); + const Identifier& esModuleMarker = vm.propertyNames->__esModule; // Bun's intepretation of the "__esModule" annotation: // @@ -795,9 +795,23 @@ void populateESMExports( // unit tests of build tools. Happy to revisit this if users file an issue. bool needsToAssignDefault = true; - if (result.isObject()) { - auto* exports = result.getObject(); - bool hasESModuleMarker = !ignoreESModuleAnnotation && exports->hasProperty(globalObject, esModuleMarker); + if (auto* exports = result.getObject()) { + bool hasESModuleMarker = false; + if (!ignoreESModuleAnnotation) { + auto catchScope = DECLARE_CATCH_SCOPE(vm); + PropertySlot slot(exports, PropertySlot::InternalMethodType::VMInquiry, &vm); + if (exports->getPropertySlot(globalObject, esModuleMarker, slot)) { + JSValue value = slot.getValue(globalObject, esModuleMarker); + if (!value.isUndefinedOrNull()) { + if (value.pureToBoolean() == TriState::True) { + hasESModuleMarker = true; + } + } + } + if (catchScope.exception()) { + catchScope.clearException(); + } + } auto* structure = exports->structure(); uint32_t size = structure->inlineSize() + structure->outOfLineSize(); diff --git a/src/bun.js/bindings/ZigGlobalObject.cpp b/src/bun.js/bindings/ZigGlobalObject.cpp index a4598fb061..159a8459c9 100644 --- a/src/bun.js/bindings/ZigGlobalObject.cpp +++ b/src/bun.js/bindings/ZigGlobalObject.cpp @@ -2731,6 +2731,32 @@ JSC_DEFINE_HOST_FUNCTION(errorConstructorFuncCaptureStackTrace, (JSC::JSGlobalOb } extern "C" JSC::EncodedJSValue CryptoObject__create(JSGlobalObject*); +JSC_DEFINE_CUSTOM_GETTER(moduleNamespacePrototypeGetESModuleMarker, (JSGlobalObject * globalObject, JSC::EncodedJSValue encodedThisValue, PropertyName)) +{ + JSValue thisValue = JSValue::decode(encodedThisValue); + JSModuleNamespaceObject* moduleNamespaceObject = jsDynamicCast(thisValue); + if (!moduleNamespaceObject || moduleNamespaceObject->m_hasESModuleMarker != WTF::TriState::True) { + return JSC::JSValue::encode(jsUndefined()); + } + + return JSC::JSValue::encode(jsBoolean(true)); +} + +JSC_DEFINE_CUSTOM_SETTER(moduleNamespacePrototypeSetESModuleMarker, (JSGlobalObject * globalObject, JSC::EncodedJSValue encodedThisValue, JSC::EncodedJSValue encodedValue, PropertyName)) +{ + auto& vm = globalObject->vm(); + JSValue thisValue = JSValue::decode(encodedThisValue); + JSModuleNamespaceObject* moduleNamespaceObject = jsDynamicCast(thisValue); + if (!moduleNamespaceObject) { + return false; + } + auto scope = DECLARE_THROW_SCOPE(vm); + JSValue value = JSValue::decode(encodedValue); + WTF::TriState triState = value.toBoolean(globalObject) ? WTF::TriState::True : WTF::TriState::False; + RETURN_IF_EXCEPTION(scope, false); + moduleNamespaceObject->m_hasESModuleMarker = triState; + return true; +} void GlobalObject::finishCreation(VM& vm) { @@ -2839,7 +2865,9 @@ void GlobalObject::finishCreation(VM& vm) // Change prototype from null to object for synthetic modules. m_moduleNamespaceObjectStructure.initLater( [](const Initializer& init) { - init.set(JSModuleNamespaceObject::createStructure(init.vm, init.owner, init.owner->objectPrototype())); + JSObject* moduleNamespacePrototype = JSC::constructEmptyObject(init.owner); + moduleNamespacePrototype->putDirectCustomAccessor(init.vm, init.vm.propertyNames->__esModule, CustomGetterSetter::create(init.vm, moduleNamespacePrototypeGetESModuleMarker, moduleNamespacePrototypeSetESModuleMarker), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::CustomAccessor | 0); + init.set(JSModuleNamespaceObject::createStructure(init.vm, init.owner, moduleNamespacePrototype)); }); m_vmModuleContextMap.initLater( diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index 0c1edd2273..8d56ba1533 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -5147,7 +5147,7 @@ restart: if (prop == propertyNames->constructor || prop == propertyNames->underscoreProto - || prop == propertyNames->toStringTagSymbol) + || prop == propertyNames->toStringTagSymbol || (objectToUse != object && prop == propertyNames->__esModule)) return true; if (builtinNames.bunNativePtrPrivateName() == prop) @@ -5244,7 +5244,7 @@ restart: if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) { if (property == propertyNames->underscoreProto - || property == propertyNames->toStringTagSymbol) + || property == propertyNames->toStringTagSymbol || property == propertyNames->__esModule) continue; } diff --git a/src/bun.js/modules/AbortControllerModuleModule.h b/src/bun.js/modules/AbortControllerModuleModule.h index d3f198e089..5a20e6b10c 100644 --- a/src/bun.js/modules/AbortControllerModuleModule.h +++ b/src/bun.js/modules/AbortControllerModuleModule.h @@ -22,7 +22,7 @@ inline void generateNativeModule_AbortControllerModule( const auto controllerIdent = Identifier::fromString(vm, "AbortController"_s); const auto signalIdent = Identifier::fromString(vm, "AbortSignal"_s); - const Identifier esModuleMarker = builtinNames(vm).__esModulePublicName(); + const Identifier& esModuleMarker = vm.propertyNames->__esModule; exportNames.append(vm.propertyNames->defaultKeyword); exportValues.append(abortController); diff --git a/src/js/builtins/BunBuiltinNames.h b/src/js/builtins/BunBuiltinNames.h index fbec2c56bd..eabe9df617 100644 --- a/src/js/builtins/BunBuiltinNames.h +++ b/src/js/builtins/BunBuiltinNames.h @@ -21,7 +21,6 @@ namespace WebCore { using namespace JSC; #define BUN_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME(macro) \ - macro(__esModule) \ macro(_events) \ macro(abortAlgorithm) \ macro(AbortSignal) \ diff --git a/src/js/builtins/Module.ts b/src/js/builtins/Module.ts index c48de2488d..3030930313 100644 --- a/src/js/builtins/Module.ts +++ b/src/js/builtins/Module.ts @@ -76,6 +76,17 @@ export function overridableRequire(this: CommonJSModuleRecord, id: string) { // If we can pull out a ModuleNamespaceObject, let's do it. if (esm?.evaluated && (esm.state ?? 0) >= $ModuleReady) { const namespace = Loader.getModuleNamespaceObject(esm!.module); + // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype. + // Various libraries expect __esModule to be set when using ESM from require(). + // We don't want to always inject the __esModule export into every module, + // And creating an Object wrapper causes the actual exports to not be own properties. + // So instead of either of those, we make it so that the __esModule property can be set at runtime. + // It only supports "true" and undefined. Anything non-truthy is treated as undefined. + // https://github.com/oven-sh/bun/issues/14411 + if (namespace.__esModule === undefined) { + namespace.__esModule = true; + } + return (mod.exports = namespace); } } diff --git a/test/cli/run/esm-defineProperty.test.ts b/test/cli/run/esm-defineProperty.test.ts index d6289b49cc..b8053b18a7 100644 --- a/test/cli/run/esm-defineProperty.test.ts +++ b/test/cli/run/esm-defineProperty.test.ts @@ -10,15 +10,24 @@ test("defineProperty", () => { expect(Bun.inspect(CJS.default)).toBe(`{\n a: 1,\n b: 2,\n c: [Getter],\n}`); }); +import * as Self from "./esm-defineProperty.test.ts"; +export const __esModule = true; +test("shows __esModule if it was exported", () => { + expect(Bun.inspect(Self)).toBe(`Module { + __esModule: true, +}`); + expect(Object.getOwnPropertyNames(Self)).toContain("__esModule"); +}); test("arraylike", () => { - console.log(globalThis); expect(CJSArrayLike[0]).toBe(0); expect(CJSArrayLike[1]).toBe(1); expect(CJSArrayLike[2]).toBe(3); expect(CJSArrayLike[3]).toBe(4); expect(CJSArrayLike[4]).toBe(undefined); expect(CJSArrayLike).toHaveProperty("4"); + expect(Object.getOwnPropertyNames(CJSArrayLike)).not.toContain("__esModule"); + expect(Object.getOwnPropertyNames(CJSArrayLike.default)).not.toContain("__esModule"); expect(Bun.inspect(CJSArrayLike)).toBe(`Module { "0": 0, "1": 1, diff --git a/test/js/bun/resolve/esModule-annotation.test.js b/test/js/bun/resolve/esModule-annotation.test.js index bb7b8a6861..4897f265d7 100644 --- a/test/js/bun/resolve/esModule-annotation.test.js +++ b/test/js/bun/resolve/esModule-annotation.test.js @@ -20,7 +20,7 @@ describe('without type: "module"', () => { }); // The module namespace object will not have the __esModule property. - expect(WithoutTypeModuleExportEsModuleAnnotationNoDefault).not.toHaveProperty("__esModule"); + expect(WithoutTypeModuleExportEsModuleAnnotationNoDefault.__esModule).toBeUndefined(); }); test("exports.default = true; exports.__esModule = true;", () => { @@ -48,7 +48,7 @@ describe('with type: "module"', () => { }); // The module namespace object WILL have the __esModule property. - expect(WithTypeModuleExportEsModuleAnnotationNoDefault).toHaveProperty("__esModule"); + expect(WithTypeModuleExportEsModuleAnnotationNoDefault.__esModule).toBeTrue(); }); test("exports.default = true; exports.__esModule = true;", () => { diff --git a/test/js/bun/resolve/esModule.test.ts b/test/js/bun/resolve/esModule.test.ts new file mode 100644 index 0000000000..8130a1cbe8 --- /dev/null +++ b/test/js/bun/resolve/esModule.test.ts @@ -0,0 +1,27 @@ +import { test, expect } from "bun:test"; + +const Self = await import("./esModule.test.ts"); + +test("__esModule defaults to undefined", () => { + expect(Self.__esModule).toBeUndefined(); +}); + +test("__esModule is settable", () => { + Self.__esModule = true; + expect(Self.__esModule).toBe(true); + Self.__esModule = false; + expect(Self.__esModule).toBe(undefined); + Self.__esModule = true; + expect(Self.__esModule).toBe(true); + Self.__esModule = undefined; +}); + +test("require of self sets __esModule", () => { + expect(Self.__esModule).toBeUndefined(); + { + const Self = require("./esModule.test.ts"); + expect(Self.__esModule).toBe(true); + } + expect(Self.__esModule).toBe(true); + expect(Object.getOwnPropertyNames(Self)).toBeEmpty(); +});