fix: Skip wrapping CJS modules which result in invalid identifiers

nodejs / import-in-the-middle

Like `require-in-the-middle`, but for ESM import

https://www.npmjs.com/package/import-in-the-middle

Apache License 2.0

52 stars 20 forks source link

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

Open timfish opened 1 week ago

timfish commented 1 week ago

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!

timfish commented 6 days ago

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 commented 6 days ago

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.