-
Notifications
You must be signed in to change notification settings - Fork 31
fix: Account for module.exports
export
#197
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
base: main
Are you sure you want to change the base?
Conversation
f07f8cf
to
1ca82b3
Compare
module.exports
exportsmodule.exports
export
1ca82b3
to
63645a5
Compare
63645a5
to
2a4f296
Compare
module.exports
exportmodule.exports
export
#115 is the more robust fix for this |
const objectKey = n === 'module.exports' ? JSON.stringify(n) : n | ||
const objectKeyWithAccess = n === 'module.exports' ? `[${objectKey}]` : `.${objectKey}` |
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.
Can we use the object[bracket] syntax for all exports?
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.
I figured we had done the dot syntax for some reason, so I elected to not change it, but now looking at it further, we probably can. Let me test something else out.
#115 excludes any library with "invalid" exports from being wrapped by iitm. I think there might be a way to use the approach in this PR to actually export these and keep the library instrument'able by iitm... |
I think if we just use the object syntax, we can do this. Let me push up another commit with what I'm thinking. |
For the intermediate variables we currently use something containing the export name which causes issues but there's nothing stopping us from using something unique like a counter Is it only newer versions of Node that accept exporting names with strings like that? I suspect we might need a legacy output mode for older Node versions... |
I think so, the tests were breaking in node 12 and 14, so I pinned to node 16. |
fixes #196