Skip to content
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

feat(compartment-mapper): full support of __esModule #1149

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

naugtur
Copy link
Member

@naugtur naugtur commented Apr 11, 2022

Fixes: #927

@kriskowal
Copy link
Member

  1. This looks correct to me.
  2. This is horrible.

I’m content to follow your lead on this behavior. If that means holding this back until we have a motivating case, I’m fine with that.

@naugtur
Copy link
Member Author

naugtur commented Apr 12, 2022

I woke up with a case that's likely to break it. None of the transpilers would do it, but this won't work:

module.exports=()=>{•••}
module.exports.__esModule=true
module.exports.default=something

The function becomes unreachable if required because we're gonna return the module record namespace because default is taken over by esmodule.

Transpiled code will not create this type of abomination though. (they're into other stuff 😉)

This is in theory a breaking change, so if we ever want to introduce it, we better introduce it early. But! I want to at least find proof it's needed for anything.

@naugtur
Copy link
Member Author

naugtur commented May 4, 2022

This solution doesn't stand up to the test of real ecosystem examples. Adding it has not improved test results on real packages and has broken a few.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account for ESM/CJS compat mode __esModule
2 participants