fix: ensure the hooked module exports has @@toStringTag property

[trentm]

The README example says the Hook callback exported arg is
"effectively import * as exported from ${url}".
https://tc39.es/ecma262/#sec-module-namespace-objects specs that
a Module Namespace Object has a @@toStringTag property with value
"Module" and no constructor.

Fixes: #57
Obsoletes: #64

---

This behaviour changed with the changes in #43 when the register(...)'d namespace changed from using an actual imported module object to using a plain object with module properties copied over to it:
https://github.com/DataDog/import-in-the-middle/pull/43/files#diff-e69a24a4c3746fa1ee96a78e12bb12d2dd4eb6e4cacbced2bf1f4084952681d9L130-R208
I suspect that was an unintentional change.

The main question here is whether you would like import-in-the-middle to promise this: that the exported namespace returned to the Hook callback mimics this aspect of import * as exported from '...'.

As links to #57 show, this would help the OpenTelemetry JS project. I started using exported[Symbol.toStringTag] in OpenTelemetry instrumentations a while back as a way to handle differences in instrumentating a module based on whether it was being used from ESM code vs CommonJS code. This is convenient because OpenTelemetry core instrumentation code uses the same hook function for require-in-the-middle and import-in-the-middle hooks. It also seemed reasonable given the Module Namespace Object spec entry. However, I grant that the exported arg need not be a Module Namespace Object.

---

Assume you are willing to accept this, a note on my implementation:

I chose to explicitly add the @@toStringTag property because:

• it is more explicit
• the for (const k of Object.getOwnPropertySymbols(primary)) { ... } alternative (proposed in @@toStringTag property not present on modules passed to hookFn  #57 and added module property symbols #64) will only ever include the @@toStringTag. Assuming my read of the https://tc39.es/ecma262/#sec-module-namespace-objects and https://tc39.es/ecma262/#sec-module-namespace-exotic-objects sections is correct, the module object will only ever have string keys (for the "export"s), plus the one @@toStringTag property.
• the @@toStringTag property should not be enumerable (i.e. Object.getOwnPropertyDescriptor(exported, Symbol.toStringTag).enumerable === false). The other implementation does not persist that descriptor value.

[lforst]

I'd love to see this land.

Sorry for the ping but we'd love for the PR to not land in limbo. @khanayan123 @bengl is this on your radar?

[chentsulin]

Is there any chance to move this forward? Thanks for all your efforts.

[bengl]

This looks good for now. My only concerns are:

1. What happens if a module exports @@toStringTag? Is the behaviour there consistent with what happens without our loader? It seems like yes, because non-string-keys don't end up on the namespace object. I think we're fine here.
2. Are symbol exports valid in ESM at all? If they are, we'll have a problem if we continue relying on the namespace object. We may need to re-think IITM in that case.

I consider concern 1 to be resolved and concern 2 to be too big to handle in this PR, so it's all good from my end. If we're passing everywhere I'll merge this.

[bengl]

@trentm looks like just a lint fix needed now.

[trentm]

2. Are symbol exports valid in ESM at all? If they are, we'll have a problem if we continue relying on the namespace object. We may need to re-think IITM in that case.

@bengl Thanks for the review.

My inexperienced read of the https://tc39.es/ecma262/#sec-module-namespace-exotic-objects is that an ES module can only ever export string-keyed properties.

[trentm]

Thanks!

[chentsulin]

Thanks! @bengl @khanayan123 Are we ready to release a v1.7.4 version with this fix?

[AbhiPrasad]

Would like to bump again - @bengl @khanayan123 is it possible to get an import-in-the-middle release with this fix? Is there something we can help with to move it along faster?

cc @mabdinur