fix: Skip wrapping CJS modules which result in invalid identifiers by timfish · Pull Request #115 · nodejs/import-in-the-middle

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

    "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

    "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!

timfish · 2025-05-08T19:42:17Z

In the Diagnostics WorkGroup meeting we decided that this PR has enough approvals and consensus that we can merge it.

Unfortunatly now a test fails!

timfish · 2025-06-11T20:54:40Z

Closing in favour of #198

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 Qard, bengl and jsumners-nr 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

Closed

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

Closed

mhdawson mentioned this pull request Dec 9, 2024

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

Closed

mhdawson mentioned this pull request Dec 30, 2024

Node.js Diagnostics WorkGroup Meeting 2025-01-02 nodejs/diagnostics#650

Closed

mhdawson mentioned this pull request Jan 20, 2025

Node.js Diagnostics WorkGroup Meeting 2025-01-23 nodejs/diagnostics#651

Closed

mhdawson mentioned this pull request Feb 10, 2025

Node.js Diagnostics WorkGroup Meeting 2025-02-13 nodejs/diagnostics#653

Closed

mhdawson mentioned this pull request Apr 14, 2025

Node.js Diagnostics WorkGroup Meeting 2025-04-17 nodejs/diagnostics#657

Closed

mhdawson mentioned this pull request May 5, 2025

Node.js Diagnostics WorkGroup Meeting 2025-05-08 nodejs/diagnostics#658

Closed

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

c3ddb47

timfish removed the diag-agenda label May 8, 2025

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

3a3810d

This was referenced May 28, 2025

yargs-parser and cliui integration tests failing #196

Closed

fix: Account for module.exports export #197

Closed

timfish marked this pull request as draft May 29, 2025 15:55

AbhiPrasad mentioned this pull request Jun 11, 2025

fix: Account for invalid identifiers #198

Merged

timfish closed this Jun 11, 2025

Conversation

timfish commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners-nr left a comment

Choose a reason for hiding this comment

Uh oh!

jsumners-nr Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

jsumners-nr Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

timfish Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

jsumners-nr Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

Qard Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

timfish Aug 20, 2024

Choose a reason for hiding this comment

Uh oh!

timfish commented Jul 1, 2024

Uh oh!

jsumners-nr commented Jul 1, 2024

Uh oh!

MattIPv4 commented Aug 12, 2024

Uh oh!

Qard Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

trentm commented Oct 17, 2024

Uh oh!

trentm commented Oct 17, 2024

Uh oh!

trentm commented Oct 17, 2024

Uh oh!

trentm left a comment

Choose a reason for hiding this comment

Uh oh!

timfish commented May 8, 2025

Uh oh!

timfish commented Jun 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

timfish commented Jun 25, 2024 •

edited

Loading