Allow string names in exports that aren't valid JavaScript variable names

[jackjennings]

I ran into an issue with a package in the wild when using the ESM loader with the node-machine-id library.

Minimal repro:

node --loader=dd-trace/loader-hook.mjs

> import("node-machine-id")
> file:///directus/node_modules/.pnpm/node-machine-id@1.1.12/node_modules/node-machine-id/dist/index.js?iitm=true:14
let $electron-machine-id = namespace.electron-machine-id
             ^
Uncaught SyntaxError: Unexpected token '-'

The node-machine-id package apparently uses an export from a UMD module that includes -. This causes an issue in the code generated by import-in-the-middle, as it assumes the export uses a name that is also a valid variable name.

Naming an export using an arbitrary string appears to be valid syntax according to the documentation on MDN: https://developer.mozilla.org/en-US/docs/web/javascript/reference/statements/export#syntax

The proposed proof-of-concept change uses ~a base64~ hexadecimal encoding to create a valid variable name, and then re-exports using the original name. Open to other ideas about how to create valid variable names.

Some syntax in the generated module is also updated to use string object access instead of dot access to allow access based on arbitrary string names.

~Note that I haven't included a meaningful test here without knowing how to best extend the existing test coverage. I have included an example export that fails under the current implementation.~ Test added.

[jackjennings]

Took a stab at writing a real test for this and getting some failures on the setter bits. I'll revisit, but if any maintainers are looking at this and can provide an up or down on whether this might be accepted I can save myself some time fiddling with it.

[jackjennings]

Actually: test completed and pushed, and removed dependency on btoa in lieu of hex encoding via Buffer. Still open to other ideas…

[Qard]

@khanayan123 This will likely conflict with your AST parsing rewrite so should see if this can be adapted somehow. 🤔

[jackjennings]

@khanayan123 how imminent is that rewrite? This issue is currently blocking me from using dd-trace on Node 18 for an open-source application that I'm hosting myself and would like to instrument.

[khanayan123]

@khanayan123 how imminent is that rewrite? This issue is currently blocking me from using dd-trace on Node 18 for an open-source application that I'm hosting myself and would like to instrument.

Hopefully in a couple of weeks. Going to discuss with @bengl to see if we can merge in your changes first.

[jackjennings]

Thanks — feel free to co-opt the work in this PR if you like; if I get a moment I'll try to figure out how / if Node 12 and 14 can be supported.

[jackjennings]

@khanayan123 I slept on this and I actually don't think that it's pressing enough for me to want you to spend any time on this PR if the rewrite is a few weeks out.

[bengl]

Node.js 12 and 14 cannot be supported since on those versions, exports do need to be valid variable names. We can merge this PR as-is if we do a major release that drops support for Node.js v14 and lower, but we don't currently have plans for that, so for now, in order to land this, we'll need to see this code gated by a version check.

[jackjennings]

@bengl I'm resurrecting this PR because it seems like the larger refactor is still in the works? I left a few comments above, but can drive this work to conclusion with a bit of guidance if you think that it's still valuable.

[timfish]

I recently opened an issue (#94) but missed this PR.

I think the simplest solution to fix this without a breaking change is to not wrap modules that have exports that aren't valid identifiers. This means that users won't be able to Hook these but that's probably not a great loss 🤷‍♂️

[timfish]

I've opened #115 which skips wrapping CJS files with exports that aren't valid identifiers.

Since libraries with exports like this are rare, I suspect it's highly unlikely that anyone will want to hook them with iitm.