Skip to content

Commit

Permalink
fix: ensure the hooked module exports has @@toStringTag property (#66)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
trentm authored Apr 17, 2024
1 parent c3c2c52 commit 20f2995
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
3 changes: 2 additions & 1 deletion hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ import { register } from '${iitmURL}'
${imports.join('\n')}
const namespaces = [${namespaces.join(', ')}]
const _ = {}
// Mimic a Module object (https://tc39.es/ecma262/#sec-module-namespace-objects).
const _ = Object.create(null, { [Symbol.toStringTag]: { value: 'Module' } })
const set = {}
const primary = namespaces.shift()
Expand Down
25 changes: 25 additions & 0 deletions test/hook/module-toStringTag.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2.0 License.
//
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.

import Hook from '../../index.js'
import { foo as fooMjs } from '../fixtures/something.mjs'
import { foo as fooJs } from '../fixtures/something.js'
import { strictEqual, deepStrictEqual } from 'assert'

let hookedExports

Hook((exports, name) => {
hookedExports = exports
})

strictEqual(fooMjs, 42)
strictEqual(fooJs, 42)

strictEqual(hookedExports[Symbol.toStringTag], 'Module')
deepStrictEqual(Object.getOwnPropertyDescriptor(hookedExports, Symbol.toStringTag), {
value: 'Module',
enumerable: false,
writable: false,
configurable: false
})

0 comments on commit 20f2995

Please sign in to comment.