added module property symbols #64
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
khanayan123
reviewed
Mar 19, 2024
|
To link the two for easy finding, seems this is intended to close #57 as it matches the suggested change |
… to createHook" This reverts commit 878fadb.
|
I opened an alternative PR at #66 |
bengl
pushed a commit
that referenced
this pull request
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.
|
#66 has gone in, so this could probably be closed now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.