-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Updated shimmer to handle instrumenting named and default exports of CommonJS modules in ESM #1894
Conversation
2d6d1fb
to
50ef671
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1894 +/- ##
=======================================
Coverage 96.87% 96.88%
=======================================
Files 209 209
Lines 39774 39863 +89
=======================================
+ Hits 38533 38622 +89
Misses 1241 1241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
50ef671
to
a31eca0
Compare
a31eca0
to
e06f66e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and much more readable. @bizob2828 should weigh in as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good. I'm not sure of the impact of proxying proxies but appears to work with our versioned tests. I do have a few comments and small things that need addressed
e06f66e
to
96a50cf
Compare
Frankly, it's a terrible idea. But we need to double up the decoration for CJS imported as ESM, otherwise we have to educate users that the imports do not work as they expect. I do think the IITM team is open to moving this work into the IITM module. So I'm going to work up a patch over there that will do the same thing and see if we can then simplify here. But I am convinced we need the symbol added in this implementation and I do not think they are open to that bit. |
This PR resolves #1821.