Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@@toStringTag property not present on modules passed to hookFn #57

Closed
pichlermarc opened this issue Jan 23, 2024 · 0 comments · Fixed by #66
Closed

@@toStringTag property not present on modules passed to hookFn #57

pichlermarc opened this issue Jan 23, 2024 · 0 comments · Fixed by #66

Comments

@pichlermarc
Copy link

Expected Behavior

Using Hook, the module passed to hookFn includes @@toStringTag property as was the case up until [email protected]

Actual Behavior

Using Hook, the module passed to hookFn does not include the @@toStringTag property.

Steps to Reproduce the Problem

  1. npm init
  2. npm install --save-exact [email protected] (used as an example)
  3. npm install --save-exact [email protected]
  4. create index.mjs
// index.mjs
import Hook from 'import-in-the-middle'
import * as koa from 'koa';

Hook(['koa'], (exported, name, baseDir) => {
  // Expect "Module"
  if(exported[Symbol.toStringTag] !== "Module"){
      throw new Error('Expected module')
  }
})

console.log('Everything as expected');
  1. node --loader=import-in-the-middle/hook.mjs index.mjs This fails

However, doing the same with [email protected] works as expected:

  1. npm install --save-exact [email protected]
  2. node --loader=import-in-the-middle/hook.mjs index.mjs This succeeds

Additional info

I'm not certain that this is safe, but adding this to the generated code in hook.js after L312 includes the property from the primary namespace and seems to fix the issue.

for (const k of Object.getOwnPropertySymbols(primary)) {
   _[k] = primary[k]
 }

Specifications

  • Version: Node.js v18.19.0
  • Platform: Ubuntu 22.04
trentm added a commit to trentm/import-in-the-middle that referenced this issue Mar 28, 2024
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: nodejs#57
@bengl bengl closed this as completed in #66 Apr 17, 2024
bengl pushed a commit that referenced this issue Apr 17, 2024
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](open-telemetry/opentelemetry-js-contrib#1694 (comment))
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 #57 and #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant