Fix duplicate default exports #53
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
|
Moving back to draft as the issue isn't quite solved. It is possible the wrong default gets exported. In the case of |
jsumners-nr
force-pushed
the
default-crash
branch
from
8bee24b to
dd5d07b
Compare
jsumners-nr
force-pushed
the
default-crash
branch
5 times, most recently
from
b5df7bb to
449dd2b
Compare
jsumners-nr
force-pushed
the
default-crash
branch
from
449dd2b to
edff49d
Compare
bengl
approved these changes
Jan 8, 2024
| if (matches === null) return | ||
| return `_['${matches[1]}'] = ${matches[2]}.default` | ||
| }) | ||
| .filter(s => s) |
There was a problem hiding this comment.
We'll probably want to eventually rework this for performance, but for now, can you can you add a double-exclamation to make it clear that it's filtering on truthiness, and therefore eliminating undefined holes?
Suggested change
| .filter(s => s) | |
| .filter(s => !!s) |
There was a problem hiding this comment.
In my opinion, the !! construct is very difficult to read. Would a comment suffice?
david-luna
pushed a commit
to elastic/apm-agent-nodejs
that referenced
this pull request
Jan 22, 2024
- Fixes node 20 reexports nodejs/import-in-the-middle#30 - Fixes duplicate default exports nodejs/import-in-the-middle#53
PeterEinberger
pushed a commit
to fpm-git/apm-agent-nodejs
that referenced
this pull request
Aug 20, 2024
) - Fixes node 20 reexports nodejs/import-in-the-middle#30 - Fixes duplicate default exports nodejs/import-in-the-middle#53
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.
Ref: newrelic/node-newrelic#1920
👋 my last "fix" (#43) broke in a new way thanks to the helpful ESM allowance of exportingdefaultmultiple times. This PR resolves the issue. I suspect there could be further problems with the exports since I have no idea how exportingdefaultmultiple 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 adefaultexport and munge the names of any subsequent ones.I have since learned that ESM doesn't actually allow exporting
defaultmultiple times. Transitivedefaultexports get mapped to some other name for the module that has imported them, and only the directly importeddefaultgets exported. Still, we have to handle figuring out whichdefaultis 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.