Support export * from 'module' ESM syntax
#43
Merged
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
|
Putting this in draft because there is another case that needs to be handled. |
|
Okay, I can't promise this is going to work for everything, but it at least clear the reproduction case provided in #31. |
This change adds support for modules that export entities through the `export * from 'module'` ESM syntax. This resolves issue DataDog#31.
jsumners-nr
force-pushed
the
star-export
branch
from
bf62475
to
88d15b6
Compare
bengl
requested changes
Dec 12, 2023
bengl
approved these changes
Dec 13, 2023
This was referenced Dec 13, 2023
Merged
bengl
pushed a commit
that referenced
this pull request
Jan 8, 2024
Ref: newrelic/node-newrelic#1920 <strike>👋 my last "fix" (#43) broke in a new way thanks to the helpful ESM allowance of exporting `default` multiple times. This PR resolves the issue. I suspect there could be further problems with the exports since I have no idea how exporting `default` multiple times is meant to be interpreted, but I suspect any such issues will be an extreme edge case. The only other thing I can think of to do here is to somehow track that we already have a `default` export and munge the names of any subsequent ones.</strike> ----- I have since learned that ESM doesn't actually allow exporting `default` multiple times. Transitive `default` exports get mapped to some other name for the module that has imported them, and only the directly imported `default` gets exported. Still, we have to handle figuring out which `default` is the right one and construct our shim namespace accordingly. ----- 2023-01-03: this can only be supported on Node versions where we parse the modules with an AST parser and interpret the code manually. So I have updated the test suite for this feature to only run on such versions. --------- Co-authored-by: James Sumners <git@zifbang.com>
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.
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.
This change adds support for modules that export entities through the
export * from 'module'ESM syntax. This resolves issue #31.