oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
74.41k stars 2.78k forks source link

Shimmer patching breaks on bundled bun code #13165

Open chargome opened 3 months ago

chargome commented 3 months ago

What version of Bun is running?

1.1.22-canary.96+df33f2b2a

What platform is your computer?

Darwin 23.5.0 arm64 arm

What steps can reproduce the bug?

Hi! We're running into issues for bun users that want to use Sentry's express instrumentation on bun.

The issue seems to be occur whenever shimmer patches the request module in a bundled bun file: TypeError: Cannot replace module namespace object's binding with configurable attribute

I have created a minimal reproduction repo with instructions in the readme: https://github.com/chargome/ex.bun-sentry-express

What is the expected behavior?

No response

What do you see instead?

No response

Additional information

No response

bnussman commented 2 months ago

For context, here's what happens when you try to load Sentry using --preload with the stacktrace

➜  api git:(main) ✗ pnpm dev

> @beep/api@0.0.1 dev /home/banks/Development/beep/api
> bun run --preload ./instrument.ts src/index.ts

Sentry Logger [log]: Initializing Sentry: process: 33583, thread: main.
Sentry Logger [log]: Integration installed: InboundFilters
Sentry Logger [log]: Integration installed: FunctionToString
Sentry Logger [log]: Integration installed: LinkedErrors
Sentry Logger [log]: Integration installed: RequestData
Sentry Logger [log]: Integration installed: Console
Sentry Logger [log]: Integration installed: Http
Sentry Logger [log]: Integration installed: NodeFetch
Sentry Logger [log]: Integration installed: OnUncaughtException
Sentry Logger [log]: Integration installed: OnUnhandledRejection
Sentry Logger [log]: Integration installed: ContextLines
Sentry Logger [log]: Integration installed: Context
Sentry Logger [log]: Integration installed: Modules
Sentry Logger [log]: Integration installed: BunServer
Sentry Logger [log]: Running in CommonJS mode.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for diag v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for trace v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for context v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for propagation v1.9.0.
Sentry Logger [debug]: @opentelemetry/instrumentation-http Applying instrumentation patch for nodejs core module on require hook {
  module: "http",
}
 9 | 
10 | // Sets a property on an object, preserving its enumerability.
11 | // This function assumes that the property is already writable.
12 | function defineProperty (obj, name, value) {
13 |   var enumerable = !!obj[name] && obj.propertyIsEnumerable(name)
14 |   Object.defineProperty(obj, name, {
     ^
TypeError: Cannot replace module namespace object's binding with configurable attribute
      at defineProperty (native:1:1)
      at defineProperty (/home/banks/Development/beep/node_modules/shimmer/index.js:14:10)
      at wrap (/home/banks/Development/beep/node_modules/shimmer/index.js:71:5)
      at /home/banks/Development/beep/node_modules/@opentelemetry/instrumentation-http/build/src/http.js:80:32
      at /home/banks/Development/beep/node_modules/@opentelemetry/instrumentation/build/src/platform/node/RequireInTheMiddleSingleton.js:67:27
      at patchedRequire (/home/banks/Development/beep/node_modules/require-in-the-middle/index.js:310:28)
      at /home/banks/Development/beep/api/node_modules/nodemailer/lib/fetch/index.js:20:1
      at anonymous (native:1:1)
      at patchedRequire (/home/banks/Development/beep/node_modules/require-in-the-middle/index.js:217:27)
      at /home/banks/Development/beep/api/node_modules/nodemailer/lib/shared/index.js:607:9
jahusa02 commented 1 month ago

+1

Jarred-Sumner commented 2 weeks ago

When you use --preload on output built via bun build, that creates two different copies of the same code. This will cause all sorts of strange issues, like what you're seeing

It sounds like what we should do is support --preload in bun build --target=bun.

doepnern commented 2 weeks ago

It also seems to be happening in our project when not using --preload and just importing the ./instrument.ts (I could not reproduce it in a small example though)

Tunanika commented 2 weeks ago

Hello, when I use bun run dev I can run the application without problems. However, when we run the application using bun run build which for our case is bun build index.mts --target bun --outdir ./out --sourcemap=external. We face problems when we run the app using bun run out/index.js.

ps: I am not using --preload on bunconfig

The output is:

 bun run out/index.js                                                                                                                                                                                                                                
 9 | 
10 | // Sets a property on an object, preserving its enumerability.
11 | // This function assumes that the property is already writable.
12 | function defineProperty (obj, name, value) {
13 |   var enumerable = !!obj[name] && obj.propertyIsEnumerable(name)
14 |   Object.defineProperty(obj, name, {
     ^
TypeError: Cannot replace module namespace object's binding with configurable attribute
      at defineProperty (native:1:1)
Tunanika commented 2 weeks ago

I have noticed that on @chargome 's reproduction repository the project runs without any problems since sentry wasn't provided with a dsn. However, when sentry is provided with a dsn the above problem with shimmer occurs.