fix: Skip wrapping CJS modules which result in invalid identifiers

[timfish]

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!

[jsumners-nr]

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

[jsumners-nr]

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

[jsumners-nr]

Bump

[timfish]

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.

[jsumners-nr]

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.

[Qard]

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

[timfish]

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

[timfish]

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]

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]

👋 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]

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

[trentm]

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]

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]

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!