Unexpected closing brace error from import-in-the-middle

FozzieHi commented 2 weeks ago

Is there an existing issue for this?

[X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[X] I have reviewed the documentation https://docs.sentry.io/
[X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.15.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup/Reproduction Example

No response

Steps to Reproduce

Branching off this issue, when running import-in-the-middle v1.8.1, I'm getting the following error:

Error: 'import-in-the-middle' failed to wrap 'file:///app/node_modules/zlib-sync/index.js'
    at getSource (/app/node_modules/import-in-the-middle/hook.js:304:21)
    at async load (/app/node_modules/import-in-the-middle/hook.js:319:26)
    at async nextLoad (node:internal/modules/esm/hooks:866:22)
    at async Hooks.load (node:internal/modules/esm/hooks:449:20)
    at async handleMessage (node:internal/modules/esm/worker:196:18) {
  cause: Error: Unexpected closing brace.
    at @:4:142
      at parseSource (/app/node_modules/cjs-module-lexer/lexer.js:185:17)
      at parseCJS (/app/node_modules/cjs-module-lexer/lexer.js:43:5)
      at getCjsExports (/app/node_modules/import-in-the-middle/lib/get-exports.js:37:20)
      at getExports (/app/node_modules/import-in-the-middle/lib/get-exports.js:100:12)
      at async /app/node_modules/import-in-the-middle/lib/get-exports.js:48:28
      at async Promise.all (index 0)
      at async getCjsExports (/app/node_modules/import-in-the-middle/lib/get-exports.js:40:5)
      at async processModule (/app/node_modules/import-in-the-middle/hook.js:131:23)
      at async getSource (/app/node_modules/import-in-the-middle/hook.js:269:25)
      at async load (/app/node_modules/import-in-the-middle/hook.js:319:26) {
    loc: 973
  }
}

This causes this from discord.js:

WebSocketShard: Compression is enabled but zlib-sync is not installed, falling back to identify compress

It still occurs with import-in-the-middle v1.9.0, but as a warning, this is the output with --trace-warnings enabled:

WebSocketShard: Compression is enabled but zlib-sync is not installed, falling back to identify compress
(node:1) Warning: Error: 'import-in-the-middle' failed to wrap 'file:///app/node_modules/zlib-sync/index.js'
    at getSource (/app/node_modules/import-in-the-middle/hook.js:366:21)
    at async load (/app/node_modules/import-in-the-middle/hook.js:381:26)
    ... 2 lines matching cause stack trace ...
    at async handleMessage (node:internal/modules/esm/worker:196:18) {
  cause: Error: Failed to parse 'file:///app/node_modules/zlib-sync/index.js'
      at getExports (/app/node_modules/import-in-the-middle/lib/get-exports.js:118:17)
      at async processModule (/app/node_modules/import-in-the-middle/hook.js:172:23)
      ... 3 lines matching cause stack trace ...
      at async Hooks.load (node:internal/modules/esm/hooks:449:20)
      at async handleMessage (node:internal/modules/esm/worker:196:18) {
    cause: Error: Failed to parse 'file:///app/node_modules/zlib-sync/build/Release/zlib_sync.node'
        at getExports (/app/node_modules/import-in-the-middle/lib/get-exports.js:118:17)
        at async /app/node_modules/import-in-the-middle/lib/get-exports.js:51:28
        ... 4 lines matching cause stack trace ...
        at async getSource (/app/node_modules/import-in-the-middle/hook.js:331:25)
        at async load (/app/node_modules/import-in-the-middle/hook.js:381:26)
        at async nextLoad (node:internal/modules/esm/hooks:866:22)
        at async Hooks.load (node:internal/modules/esm/hooks:449:20) {
      cause: [Error]
    }
  }
}
    at emitWarning (/app/node_modules/import-in-the-middle/hook.js:156:11)
    at getSource (/app/node_modules/import-in-the-middle/hook.js:368:9)
    at async load (/app/node_modules/import-in-the-middle/hook.js:381:26)
    at async nextLoad (node:internal/modules/esm/hooks:866:22)
    at async Hooks.load (node:internal/modules/esm/hooks:449:20)
    at async handleMessage (node:internal/modules/esm/worker:196:18)

Expected Result

N/A

Actual Result

N/A

AbhiPrasad commented 2 weeks ago

Hey @FozzieHi thanks for the new issue. I'll let @timfish handle taking a look at this, and it's also being tracked under our main umbrella issue for all instrumentation problems: https://github.com/getsentry/sentry-javascript/issues/12806

timfish commented 2 weeks ago

Handling of native modules needs improving in import-in-the-middle and I've opened an issue to address this but currently this is just logging a warning when it's encountered and it shouldn't impact @sentry/node.

FozzieHi commented 2 weeks ago

Thanks @timfish, if it is just a warning, I'm wondering why it's causing discord.js to log this out:

WebSocketShard: Compression is enabled but zlib-sync is not installed, falling back to identify compress

I've taken a brief look at the discord.js code, and it looks like it is affecting this line, which if it's falsy, causes the message to be logged out:

const getZlibSync = lazy(async () => import('zlib-sync').then((mod) => mod.default).catch(() => null));

https://github.com/discordjs/discord.js/blob/311aaf26058c7a95f6d973686e49818c9ecb0fb1/packages/ws/src/ws/WebSocketShard.ts#L177

timfish commented 1 week ago

A fix was release in import-in-the-middle@1.9.1. Could you try it out and see if it fixes this issue?

You can either delete your lock file and reinstall or npm/yarn update import-in-the-middle

FozzieHi commented 1 week ago

That works, thanks for your help on this! As an aside, do we know why the warning actually affected discord.js in this instance? If these sort of warnings can cause application issues, shouldn't it be changed to an error?

timfish commented 1 week ago

import-in-the-middle instruments every ES module and previosly it threw errors when failing to instrument a file which terminates the process and leaves users unable to use the SDK.

We changed import-in-the-middle to catch these errors with the only downside being that these specific modules cannot be instrumented. For example in your case, the Sentry SDK doesn't instrument zlib-sync, so it doesn't matter:

Error: 'import-in-the-middle' failed to wrap 'file:///app/node_modules/zlib-sync/index.js'

Catching these errors helps SDK users get on with their work but we still log them as warnings because then users open helpful issues like this one which let us know where import-in-the-middle might have bugs or need improvement!

FozzieHi commented 1 week ago

Right, but in this case the warning in import-in-the-middle did cause this variable to somehow become falsy, is it worth opening a new issue on the import-in-the-middle repo to investigate more?

const getZlibSync = lazy(async () => import('zlib-sync').then((mod) => mod.default).catch(() => null));

timfish commented 1 week ago

If the issue is still reproducible with v1.9.1, I would open a new issue.

getsentry / sentry-javascript