Skip to content

Commit 20f2995

Browse files
authored
fix: ensure the hooked module exports has @@toStringTag property (#66)
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.
1 parent c3c2c52 commit 20f2995

File tree

2 files changed

+27
-1
lines changed

2 files changed

+27
-1
lines changed

hook.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,8 @@ import { register } from '${iitmURL}'
303303
${imports.join('\n')}
304304
305305
const namespaces = [${namespaces.join(', ')}]
306-
const _ = {}
306+
// Mimic a Module object (https://tc39.es/ecma262/#sec-module-namespace-objects).
307+
const _ = Object.create(null, { [Symbol.toStringTag]: { value: 'Module' } })
307308
const set = {}
308309
309310
const primary = namespaces.shift()

test/hook/module-toStringTag.mjs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2.0 License.
2+
//
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
4+
5+
import Hook from '../../index.js'
6+
import { foo as fooMjs } from '../fixtures/something.mjs'
7+
import { foo as fooJs } from '../fixtures/something.js'
8+
import { strictEqual, deepStrictEqual } from 'assert'
9+
10+
let hookedExports
11+
12+
Hook((exports, name) => {
13+
hookedExports = exports
14+
})
15+
16+
strictEqual(fooMjs, 42)
17+
strictEqual(fooJs, 42)
18+
19+
strictEqual(hookedExports[Symbol.toStringTag], 'Module')
20+
deepStrictEqual(Object.getOwnPropertyDescriptor(hookedExports, Symbol.toStringTag), {
21+
value: 'Module',
22+
enumerable: false,
23+
writable: false,
24+
configurable: false
25+
})

0 commit comments

Comments
 (0)