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

Account for ESM/CJS compat mode __esModule #927

Closed
kriskowal opened this issue Nov 3, 2021 · 9 comments · May be fixed by #1149
Closed

Account for ESM/CJS compat mode __esModule #927

kriskowal opened this issue Nov 3, 2021 · 9 comments · May be fixed by #1149
Assignees
Labels

Comments

@kriskowal
Copy link
Member

The SES and the Endo Compartment Mapper have a frail and under-tested implementation of CommonJS as a third-party static module record. These need to be more extensively tested and the difference between a CommonJS module that sets exports.__esModule distinguished from one that does now. The setter can be detected after initialization and the behavior of the default getter on the proxied exports behavior varied accordingly.

Background on compatibility mode esnext/es6-module-transpiler#85

@kriskowal kriskowal self-assigned this Nov 3, 2021
@kriskowal kriskowal added the good first issue Good for newcomers label Nov 3, 2021
@kriskowal kriskowal added endo and removed good first issue Good for newcomers labels Jan 27, 2022
@naugtur
Copy link
Member

naugtur commented Feb 17, 2022

A table from esbuild folks for reference evanw/esbuild#532 (comment)

@naugtur
Copy link
Member

naugtur commented Apr 6, 2022

Looking at the results for case 11 in the table here unless we want to be more like tsc and some transpilers, no work needs to be done other than merging
#1145

@kriskowal @kumavis please correct me if I'm wrong. I don't see any other use for __esModule and it seems to me the behavior __esModule was originally meant to trigger is the default in node (and Endo)

@kriskowal
Copy link
Member Author

I think we’re close enough. If we can get parity with Parcel, Rollup &c, it would be nice, but not at the expense of Node.js parity or our sanity.

@naugtur
Copy link
Member

naugtur commented Apr 7, 2022

The overlap between parcel and sanity is not something I'm comfortable with https://github.com/endojs/endo-e2e-tests/blob/naugtur-matrix-iteration/matrix/table.md
I may need to configure it further, but default out of the box behavior is surprising.

Anyway - we've got node compatibility and a bit more.

This is also useful for understanding the scope.
https://github.com/qnighy/node-cjs-interop
nodejs/node#40891

So __esModule set to true causes module.exports.default to be the value for the default export available in esm, because the cjs is a result of compiling an esm. BUT! Node.js refused to implement that and the ecosystem is mostly ready to work as-is.

I don't think we need to do anything else here unless we want a --esModuleInterop flag.

@naugtur
Copy link
Member

naugtur commented Apr 11, 2022

Ok, so there's a PR now. #1149

I tested it and it seems just adding this in doesn't break nor fix anything in e2e on real packages.
It's breaking some packages.

It does create a behavior difference between endo and node getting endo closer to what bundlers do.
The glitches this is potentially introducing are harder to figure out then not having it in, so I'm not sure if I want it.

I could add in some failing tests to ilustrate those.

The only sane way to introduce support for this would be in the makeThirdPartyModuleInstance because it's not supposed to have any impact on the require/cjs operation.

@kriskowal
Copy link
Member Author

My preference is not to modify third-party-module support in such a way that WASM or any other language would interact with __esModule at all. I also doubt that TC39 would accept back-wash from the __esModule hack.

Failing tests would be illuminating!

You mentioned out-of-band that there might be a path forward but it would require a new compartment hook. Can you propose such a hook? Ideally, the hook is not coupled specifically to the __esModule hack, but that a hook can be provided that addresses that usage.

My intuition is that we shouldn’t spend too much effort on __esModule. Some problems are better solved by evolving the ecosystem.

@naugtur
Copy link
Member

naugtur commented May 25, 2022

We'd have to have means for a parser implementation to provide a hook that'd then be called in module-instance to inform the process of wiring up the updaters to exports.
I started hating this idea before I finished writing the sentence.
What I actually want is the ability to put custom boolean flags in the StaticModuleRecord so I can produce different behavior depending on how the module is imported. Like the unused bytes in TCP/IP packet specification. If StaticModuleRecord had a symbol-keyed field for internal meta information, compatibility workarounds would be much easier to pull off.

I was considering putting metadata in the __esModule value itself but I doubt it'd get past review :D

As for failing tests - I'd need to find real examples and I'm struggling to find ones that actually need the __esModules logic to work correctly for their dependents. And for each such theoretical example I've already seen a counter example of a module that wouldn't work if that was implemented. Like generated code which sets __esModule but doesn't set default but people are happy to import it like the cjs module it is 😠

I'm happy leaving it unfixed until there's an issue. Where would I document that?

@kriskowal
Copy link
Member Author

For now, the compartment mapper README has a lot of notes about features, caveats, and aspirations. That’s a good place to put a note. A link to this issue would not hurt. Closing this issue until such a time is also okay.

@kriskowal
Copy link
Member Author

@naugtur Please reöpen or open a follow-up issue if the current solution needs more work.

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

Successfully merging a pull request may close this issue.

2 participants