@sentry/esbuild-plugin breaks Object.defineProperty on module.exports

[colinmcdonald22]

Environment

@sentry/esbuild-plugin: 3.1.1
esbuild: 0.24.2

Steps to Reproduce

We're using https://github.com/open-telemetry/opentelemetry-lambda for tracing in our AWS Lambda functions, along with using Sentry (and @sentry/esbuild-plugin) for error tracking.

Here's an example of how we're exporting our AWS Lambda handler:

const handler = () => {
  console.log("Hello world");
}

module.exports = { handler };

OpenTelemtry-Lambda is using Shimmer to wrap our export'd Lambda handler function, to start an OpenTelemetry trace. Shimmer does this using Object.defineProperty, as seen here: https://github.com/othiym23/shimmer/blob/master/index.js#L14C3-L14C24

When compiling this code using plain ESBuild, Shimmer is able to wrap the handler as expected. However, when compiling using ESBuild and @sentry/esbuild-plugin, the Object.defineProperty call inside Shimmer throws an exception.

I've created a small repository to reproduce the problem: https://github.com/colinmcdonald22/SentryBugRepro/tree/master

app.js is a small Lambda handler.
build.mjs invokes ESBuild, optionally enabling @sentry/esbuild-plugin based on the WITH_SENTRY environment variable
invoke.js simulates what OpenTelemetry-Lambda is doing when shimming our code.

After cloning the repo and pnpm install'ing:

pnpm run test does the following:

• Runs ESBuild without any plugins
• Runs invoke.js
  • This calls the handler function exported from app.js

This works as expected:

$ pnpm run test

> @ test C:\Users\colin\SentryBugRepro
> node build.mjs && node invoke.js

Wrapped start
Hello world
Wrapped end

Wrapped start/Wrapped end are added in invoke.js to prove shimmer is working, and Hello world is from our app's handler in app.js

Next, run WITH_SENTRY=1 pnpm run test. This is the same process as above, but with @sentry/esbuild-plugin active.

$ WITH_SENTRY=1 pnpm run test

> @ test C:\Users\colin\SentryBugRepro
> node build.mjs && node invoke.js

[sentry-esbuild-plugin] Warning: No auth token provided. Will not create release. Please set the `authToken` option. You can find information on how to generate a Sentry auth token here: https://docs.sentry.io/api/auth/
[sentry-esbuild-plugin] Warning: No auth token provided. Will not upload source maps. Please set the `authToken` option. You can find information on how to generate a Sentry auth token here: https://docs.sentry.io/api/auth/
[sentry-esbuild-plugin] Info: Sending telemetry data on issues and performance to Sentry. To disable telemetry, set `options.telemetry` to `false`.
C:\Users\colin\SentryBugRepro\node_modules\.pnpm\[email protected]\node_modules\shimmer\index.js:14
  Object.defineProperty(obj, name, {
         ^

TypeError: Cannot redefine property: handler
    at Function.defineProperty (<anonymous>)
    at defineProperty (C:\Users\colin\SentryBugRepro\node_modules\.pnpm\[email protected]\node_modules\shimmer\index.js:14:10)
    at Function.wrap (C:\Users\colin\SentryBugRepro\node_modules\.pnpm\[email protected]\node_modules\shimmer\index.js:56:3)
    at Object.<anonymous> (C:\Users\colin\SentryBugRepro\invoke.js:4:9)
    at Module._compile (node:internal/modules/cjs/loader:1358:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49

Node.js v20.16.0
 ELIFECYCLE  Test failed. See above for more details.

The Object.defineProperty inside Shimmer now throws a "Cannot redefine property". This only happens when @sentry/esbuild-plugin is installed; it does not happen with plain ESBuild.

The root of the issue seems to be that plain ESBuild generates exports that are able to be patched with Object.defineProperty, whereas ESBuild + @sentry/esbuild-plugin generates exports that are not able to be patched.

Although OpenTelemetry + Shimmer's approach is certainly brittle, I think it's reasonable to expect the Sentry ESBuild plugin doesn't change the underlying ESBuild behavior of exports being patchable.

---

A similar issue has been reported on OpenTelemetry before: aws-observability/aws-otel-lambda#99 (comment)

According to the comment I linked, code like this:

export const handler = () => {
  // removed for brevity
};

caused ESBuild to generate code that cannot be patched. However, changing it to this format:

const handler = () => {
  // removed for brevity
};
module.exports = { handler }

causes ESBuild to generate Object.defineProperty-compatible code.

I'm not familiar enough with how ESBuild functions to speculate why this makes a difference, however it seems like it might be relevant.

[chargome]

Hey @colinmcdonald22 thanks for reaching out and for the detailed rundown!

You are right, with re-exporting all modules of a file within our plugin, the exports become immutable (this is automatically done, see evanw/esbuild#1079).

So I think we're left with three options (not sure which ones are feasible in your case, this is based on the repro):

1. You patch your handler before running esbuild
2. You make a configurable copy of the export and patch that:

const originalApp = require("./dist/app");
const app = Object.create(Object.getPrototypeOf(originalApp));

// make a configurable copy here
Object.defineProperty(app, "handler", {
  configurable: true,
  writable: true,
  value: originalApp.handler,
});

shimmer.wrap(app, "handler", function (original) {
  return function () {
    console.log("Wrapped start");
    var returned = original.apply(this, arguments);
    console.log("Wrapped end");
    return returned;
  };
});

app.handler();

3. We update our plugin to somehow create mutable exports in our onLoad hook.

// ...
const configurableExports = {};
Object.entries(OriginalModule).forEach(([key, value]) => {
  Object.defineProperty(exports, key, {
    enumerable: true,
    configurable: true,
    get: () => OriginalModule[key]
  });
});
module.exports = configurableExports;

^ This feels a bit brittle tbh, wdyt @lforst ?

[colinmcdonald22]

Hey @colinmcdonald22 thanks for reaching out and for the detailed rundown!

You are right, with re-exporting all modules of a file within our plugin, the exports become immutable (this is automatically done, see evanw/esbuild#1079).

So I think we're left with three options (not sure which ones are feasible in your case, this is based on the repro):

1. You patch your handler before running esbuild
2. You make a configurable copy of the export and patch that:

const originalApp = require("./dist/app");
const app = Object.create(Object.getPrototypeOf(originalApp));

// make a configurable copy here
Object.defineProperty(app, "handler", {
configurable: true,
writable: true,
value: originalApp.handler,
});

shimmer.wrap(app, "handler", function (original) {
return function () {
console.log("Wrapped start");
var returned = original.apply(this, arguments);
console.log("Wrapped end");
return returned;
};
});

app.handler();
3. We update our plugin to somehow create mutable exports in our onLoad hook.

// ...
const configurableExports = {};
Object.entries(OriginalModule).forEach(([key, value]) => {
Object.defineProperty(exports, key, {
enumerable: true,
configurable: true,
get: () => OriginalModule[key]
});
});
module.exports = configurableExports;
^ This feels a bit brittle tbh, wdyt @lforst ?

Hi @chargome, thank you for your reply!

Regarding your proposed solutions:

1. OpenTelemetry seems to have settled on instrumenting at runtime (via a node --require flag). This is similar to Sentry's own usage of OpenTelemetry, via initializing the Sentry SDK with a --require (or --import for ESM). Instrumenting via an ESBuild plugin would certainly be desirable for our use case, but I understand why OpenTelemetry would not want to make such a change.

2. Ahh, this is where the repro I created and OpenTelemetry's actual usage diverge. I didn't think of this when creating my test case.

OpenTelemetry (and more specifically, OpenTelemetry-Lambda) loads via a --require wrapper.js. The exact usage is this: export NODE_OPTIONS="${NODE_OPTIONS} --import /opt/loader.mjs --require /opt/wrapper.js" from here: https://github.com/open-telemetry/opentelemetry-lambda/blob/main/nodejs/packages/layer/scripts/otel-handler#L5

Therefore, it's not replacing the Nodejs entrypoint, it's just running a small shim before it. Of course, from the "perspective" of a --require'd wrapper script, the Object.create(Object.getPrototypeOf(originalApp)); is just creating a copy, so overriding its property does nothing.

In order for that to work, I believe OpenTelemetry would have to actually take over the entrypoint. Instead of node --require /opt/wrapper.js myapp.js, it'd be node /opt/wrapper.js --app-entrypoint myapp.js or similar.

I believe, but might very well be incorrect, that the best way for a shim in this style (modifying a file from a --require'd wrapper, instead of replacing the entrypoint) to function is via mutable exports.

3. This is, of course, my preferred solution :) I agree that totally wrapping the entrypoint or instrumenting at compile time are valid solutions, although those are quite significant design changes impacting OpenTelemetry, and likely not feasable.

Of course, OpenTelemetry's approach of relying on mutable exports and shimming via a --require is quite niche. However, I would still hope it's a use case that @sentry/esbuild-plugin is able to support.

[chargome]

@colinmcdonald22 hmm you're right in the sense that we should not block you on doing this – I'll try to set some time aside this week to have a go at this, unless you would want to open a PR for this?

[colinmcdonald22]

@chargome this thread so far is just about the extent of my ESBuild / JavaScript knowledge unfortunately. Happy to spend some time playing with it, but definitely not confident I could resolve this

[colinmcdonald22]

hi @chargome, hope all is well! I just wanted to see if there's any thoughts on how to solve this

[chargome]

Hey @colinmcdonald22 sorry we're a bit busy atm, I did not get to this yet!

So one thing I could think of was updating our onLoad hook to iterate over the exports and make them explicitly configurable. But it I didn't test this on possible side effects yet.

e.g. something similar to:

const configurableExports = {};
Object.entries(OriginalModule).forEach(([key, value]) => {
  Object.defineProperty(exports, key, {
    enumerable: true,
    configurable: true,
    get: () => OriginalModule[key]
  });
});
module.exports = configurableExports;

[lforst]

Adding onto this: If you want my complete honesty, I would probably recommend to use sentry-cli instead of the Sentry esbuild plugin. Esbuild is really not designed to have plugins like ours and our plugin uses quite a bit of hacks to properly work.

I am actually thinking about deprecating our esbuild plugin because it just causes frustrations and people keep wasting time with it.

Here's a guide for how to upload sourcemaps with Sentry CLI: https://docs.sentry.io/platforms/javascript/sourcemaps/uploading/cli/

[colinmcdonald22]

Adding onto this: If you want my complete honesty, I would probably recommend to use sentry-cli instead of the Sentry esbuild plugin. Esbuild is really not designed to have plugins like ours and our plugin uses quite a bit of hacks to properly work.

I am actually thinking about deprecating our esbuild plugin because it just causes frustrations and people keep wasting time with it.

Here's a guide for how to upload sourcemaps with Sentry CLI: https://docs.sentry.io/platforms/javascript/sourcemaps/uploading/cli/

Understood. We've tried out the Sentry CLI and it seems to work for our use case + prevents this issue from occurring.