fix: Skip wrapping CJS modules which result in invalid identifiers #115

timfish · 2024-06-25T10:38:32Z

Closes #94

Rather than trying to find a way to only export invalid identifiers in versions of Node that support exports with names like this, this PR simply skips wrapping these modules. Since modules that use exports like this are rare, it's unlikely anyone will ever want to Hook them and if they do we can revisit this later.

@babel/helper-validator-identifier was chosen to check for invalid identifiers because the code looks good, it sees 50m+ downloads per week, it's tested, it has no dependencies and maybe most importantly, it's CommonJs!

…nvalid-indentifiers

jsumners-nr

If such modules are rare, how did the issue get exposed?

jsumners-nr · 2024-07-01T11:51:13Z

package.json

@@ -56,6 +56,7 @@
    "acorn": "^8.8.2",
    "acorn-import-attributes": "^1.9.5",
    "cjs-module-lexer": "^1.2.2",
-    "module-details-from-path": "^1.0.3"
+    "module-details-from-path": "^1.0.3",
+    "@babel/helper-validator-identifier": "^7.24.7"


I'd like to hear from @Qard or @bengl what they think about adding in this dependency.

We could vendor this code and it's around 13kB.

This package has 50m weekly downloads so there's a high chance it will already be in many users dependencies.

This package has 50m weekly downloads so there's a high chance it will already be in many users dependencies.

Maybe. But I've never seen the package before this PR. I looked through it back when this PR was opened, and I remember getting the impression that it could be overkill. So I wanted an opinion from the folks much more in tune with this code than I am.

I'm fine with it, though I wonder if we really need all that and not just a simplified subset of it.

We need all of identifier.js (12.2kB) which is most of the package.

timfish · 2024-07-01T12:58:15Z

If such modules are rare, how did the issue get exposed?

When testing the top 250+ npm packages I found webidl-conversions caused invalid wrappers to be emitted
This issue suggests wasm can result in invalid identifiers?
This open PR suggests at some point there was an issue with node-machine-id
Someone reported this issue in the Sentry repository for grpc-tools

If these are the only examples we have, it suggests it's quite uncommon but its prevalent enough for it to cause issues for users.

jsumners-nr · 2024-07-01T13:01:41Z

If such modules are rare, how did the issue get exposed?

When testing the top 250+ npm packages I found webidl-conversions caused invalid wrappers to be emitted

This issue suggests wasm can result in invalid identifiers?

This open PR suggests at some point there was an issue with node-machine-id

Someone reported this issue in the Sentry repository for grpc-tools

If these are the only examples we have, it suggests it's quite uncommon but its prevalent enough for it to cause issues for users.

Thank you.

MattIPv4 · 2024-08-12T15:36:17Z

👋 Is the blocker on this PR still waiting for reviews from specific folks? Anything that can be done to move that along? Happy to help where I can. This change is still required to fix a bug in Sentry that is blocking us, as I understand it.

Qard · 2024-08-15T01:08:02Z

package.json

@@ -56,6 +56,7 @@
    "acorn": "^8.8.2",
    "acorn-import-attributes": "^1.9.5",
    "cjs-module-lexer": "^1.2.2",
-    "module-details-from-path": "^1.0.3"
+    "module-details-from-path": "^1.0.3",
+    "@babel/helper-validator-identifier": "^7.24.7"


I'm fine with it, though I wonder if we really need all that and not just a simplified subset of it.

trentm · 2024-10-17T16:34:50Z

FWIW, it looks like this:

% cat test/hook/invalid-identifier.mjs
import * as lib from '../fixtures/invalid-identifier.js'
import { strictEqual } from 'assert'

strictEqual(typeof lib['one.two'], 'function')


% cat test/hook/invalid-identifier.mjs
import * as lib from '../fixtures/invalid-identifier.js'
import { strictEqual } from 'assert'

strictEqual(typeof lib['one.two'], 'function')


% cat test/fixtures/invalid-identifier.js
exports['one.two'] = () => console.log('b')


% cat my-setup-hook.mjs
import { register } from 'module'
import { Hook } from './index.js'

register('./hook.mjs', import.meta.url)
Hook((exports, name) => {
  console.log('my-setup-hook: hooking "%s"', name)
})


% node --import ./my-setup-hook.mjs test/hook/invalid-identifier.mjs
(node:33546) Error: 'import-in-the-middle' failed to wrap 'file:///Users/trentm/tm/import-in-the-middle2/test/fixtures/invalid-identifier.js'
(Use `node --trace-warnings ...` to show where the warning was created)
my-setup-hook: hooking "assert"
my-setup-hook: hooking "/Users/trentm/tm/import-in-the-middle2/test/hook/invalid-identifier.mjs"

With --trace-warnings:

% node --trace-warnings --import ./my-setup-hook.mjs test/hook/invalid-identifier.mjs
(node:33566) Warning: Error: 'import-in-the-middle' failed to wrap 'file:///Users/trentm/tm/import-in-the-middle2/test/fixtures/invalid-identifier.js'
    at load (/Users/trentm/tm/import-in-the-middle2/hook.js:431:21)
    at async nextLoad (node:internal/modules/esm/hooks:866:22)
    at async Hooks.load (node:internal/modules/esm/hooks:449:20)
    at async MessagePort.handleMessage (node:internal/modules/esm/worker:196:18) {
  cause: Error: 'one.two' export is not a valid identifier name
      at addSetter (/Users/trentm/tm/import-in-the-middle2/hook.js:205:13)
      at processModule (/Users/trentm/tm/import-in-the-middle2/hook.js:260:7)
      at async load (/Users/trentm/tm/import-in-the-middle2/hook.js:394:25)
      at async nextLoad (node:internal/modules/esm/hooks:866:22)
      at async Hooks.load (node:internal/modules/esm/hooks:449:20)
      at async MessagePort.handleMessage (node:internal/modules/esm/worker:196:18)
}
    at emitWarning (/Users/trentm/tm/import-in-the-middle2/hook.js:183:11)
    at load (/Users/trentm/tm/import-in-the-middle2/hook.js:433:9)
    at async nextLoad (node:internal/modules/esm/hooks:866:22)
    at async Hooks.load (node:internal/modules/esm/hooks:449:20)
    at async MessagePort.handleMessage (node:internal/modules/esm/worker:196:18)
my-setup-hook: hooking "assert"
my-setup-hook: hooking "/Users/trentm/tm/import-in-the-middle2/test/hook/invalid-identifier.mjs"

If I know about the excludes option (I don't recall if this is still considered experimental, hence why not documented) and with some trial and error, I can tweak my-setup-hook.mjs to skip that problematic module to avoid having warnings in my face all the time:

...
register('./hook.mjs', import.meta.url, {
  data: {
    exclude: [/fixtures\/invalid-identifier.js$/]
  }
})

Then, no more warning:

% node --trace-warnings --import ./my-setup-hook.mjs test/hook/invalid-identifier.mjs
my-setup-hook: hooking "assert"
my-setup-hook: hooking "/Users/trentm/tm/import-in-the-middle2/test/hook/invalid-identifier.mjs"

trentm · 2024-10-17T16:38:13Z

This doesn't need to block this PR, but my guess is this kind of message:

(node:33546) Error: 'import-in-the-middle' failed to wrap 'file:///Users/trentm/tm/import-in-the-middle2/test/fixtures/invalid-identifier.js'
(Use `node --trace-warnings ...` to show where the warning was created)

being seen by end users will result in support issues for us. :)

That might be alleviated somewhat by getting that closer to something like (taking away the words "Error" and "failed"):

(node:33546) Warning: 'import-in-the-middle' could not wrap 'file:///Users/trentm/tm/import-in-the-middle2/test/fixtures/invalid-identifier.js'
(Use `node --trace-warnings ...` to show where the warning was created)

trentm · 2024-10-17T16:41:01Z

That might be alleviated somewhat by getting that closer to something like (taking away the words "Error" and "failed"):

That would be possible with this diff:

diff --git a/hook.js b/hook.js
index b5d72f5..93664b8 100644
--- a/hook.js
+++ b/hook.js
@@ -428,7 +428,8 @@ register(${JSON.stringify(realUrl)}, _, set, ${JSON.stringify(specifiers.get(rea
         //
         // We log the error because there might be bugs in iitm and without this
         // it would be very tricky to debug
-        const err = new Error(`'import-in-the-middle' failed to wrap '${realUrl}'`)
+        const err = new Error(`'import-in-the-middle' could not wrap '${realUrl}'`)
+        err.name = 'Warning'
         err.cause = cause
         emitWarning(err)

trentm

Thanks!

fix: Skip wrapping CJS modules which result in invalid indenifiers

307a4d0

change error message

f20179c

AbhiPrasad previously approved these changes Jun 25, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix/skip-wrapping-i…

a9a5345

…nvalid-indentifiers

timfish dismissed AbhiPrasad’s stale review via a9a5345 June 29, 2024 17:25

AbhiPrasad changed the title ~~fix: Skip wrapping CJS modules which result in invalid indenifiers~~ fix: Skip wrapping CJS modules which result in invalid identifiers Jun 29, 2024

AbhiPrasad previously approved these changes Jun 29, 2024

View reviewed changes

jsumners-nr requested changes Jul 1, 2024

View reviewed changes

Merge branch 'main' into fix/skip-wrapping-invalid-indentifiers

f5c8bfd

AbhiPrasad mentioned this pull request Jul 8, 2024

Sentry Node.js ESM + OTEL instrumentation blockers - Part 3 getsentry/sentry-javascript#12806

Closed

AbhiPrasad requested review from jsumners-nr, bengl and Qard July 17, 2024 16:51

Merge branch 'main' into fix/skip-wrapping-invalid-indentifiers

d818214

timfish dismissed AbhiPrasad’s stale review via d818214 August 13, 2024 11:24

Qard approved these changes Aug 15, 2024

View reviewed changes

jsumners-nr approved these changes Aug 19, 2024

View reviewed changes

timfish added the diag-agenda label Sep 24, 2024

mhdawson mentioned this pull request Oct 8, 2024

Node.js Diagnostics WorkGroup Meeting 2024-10-10 nodejs/diagnostics#644

Open

trentm approved these changes Oct 17, 2024

View reviewed changes

mhdawson mentioned this pull request Nov 18, 2024

Node.js Diagnostics WorkGroup Meeting 2024-11-21 nodejs/diagnostics#646

Open

mhdawson mentioned this pull request Dec 9, 2024

Node.js Diagnostics WorkGroup Meeting 2024-12-12 nodejs/diagnostics#647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Skip wrapping CJS modules which result in invalid identifiers #115

fix: Skip wrapping CJS modules which result in invalid identifiers #115

timfish commented Jun 25, 2024 •

edited

Loading

jsumners-nr left a comment

jsumners-nr Jul 1, 2024

jsumners-nr Aug 13, 2024

timfish Aug 14, 2024

jsumners-nr Aug 14, 2024

Qard Aug 15, 2024

timfish Aug 20, 2024

timfish commented Jul 1, 2024

jsumners-nr commented Jul 1, 2024

MattIPv4 commented Aug 12, 2024

Qard Aug 15, 2024

trentm commented Oct 17, 2024

trentm commented Oct 17, 2024

trentm commented Oct 17, 2024

trentm left a comment

fix: Skip wrapping CJS modules which result in invalid identifiers #115

Are you sure you want to change the base?

fix: Skip wrapping CJS modules which result in invalid identifiers #115

Conversation

timfish commented Jun 25, 2024 • edited Loading

jsumners-nr left a comment

Choose a reason for hiding this comment

jsumners-nr Jul 1, 2024

Choose a reason for hiding this comment

jsumners-nr Aug 13, 2024

Choose a reason for hiding this comment

timfish Aug 14, 2024

Choose a reason for hiding this comment

jsumners-nr Aug 14, 2024

Choose a reason for hiding this comment

Qard Aug 15, 2024

Choose a reason for hiding this comment

timfish Aug 20, 2024

Choose a reason for hiding this comment

timfish commented Jul 1, 2024

jsumners-nr commented Jul 1, 2024

MattIPv4 commented Aug 12, 2024

Qard Aug 15, 2024

Choose a reason for hiding this comment

trentm commented Oct 17, 2024

trentm commented Oct 17, 2024

trentm commented Oct 17, 2024

trentm left a comment

Choose a reason for hiding this comment

timfish commented Jun 25, 2024 •

edited

Loading