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

compat: imported scripts are always handled as ESM #12648

Closed
Tracked by #12577
kt3k opened this issue Nov 4, 2021 · 6 comments · Fixed by #13553
Closed
Tracked by #12577

compat: imported scripts are always handled as ESM #12648

kt3k opened this issue Nov 4, 2021 · 6 comments · Fixed by #13553
Assignees

Comments

@kt3k
Copy link
Member

kt3k commented Nov 4, 2021

Suppose we have following scripts:

main.mjs

import "./imported.js";

imported.js

console.log(exports);

If we run this with node, it simply prints {}:

$ node main.mjs 
{}

But if we run this with deno-compat mode, then it throws like the below:

$ deno run --compat --unstable -A main.mjs
error: Uncaught ReferenceError: exports is not defined
console.log(exports);
            ^
    at file:///path/to/imported.js:1:13

The difference here is that node handles imported.js as CJS, but deno compat handles it as ESM.

Side note: This prevents deno-compat passing test/interfaces/exports.spec test case in mocha repository. related: #12577

@bartlomieju
Copy link
Member

I can look into that next week

@bartlomieju
Copy link
Member

@kt3k how burning of a need is this issue? I did some exploratory work and it seems harder than I expected to fix this problem.

@kt3k
Copy link
Member Author

kt3k commented Nov 25, 2021

4 sub test catetories are blocked by this in the mocha compatibility work. So this is very much needed, but there are some more independent issues to work on. So my work is not particularly blocked by this one

@bartlomieju
Copy link
Member

I did some research on what is the problem here - basically we don't currently have a notion of "translators" which are integral parts of Node's ESM loader. "translators" are used to transform source code of modules to compatible formats; in this case CJS module is translated into ESM (here's relevant part: https://github.com/nodejs/node/blob/42c0b2ae65e99e4774fe1d8a82a50b09b894adc7/lib/internal/modules/esm/translators.js#L168-L210).

The hard part in our case is that we got ESM loader in CLI and CJS loader in deno_std (as part of node/module.ts). There are already some discrepancies between our implementation and Node's (mainly we're not caching read package.json files in the same way as Node does), but this is significantly more challenging. I'm leaning towards the answer that we might have to have require implemented in Rust before we can tackle this problem, but before giving definitive answer I will prototype some more.

@bartlomieju
Copy link
Member

So it seems that the main integration point would have to be ProcState::load and thus it would also have to touch module graph. We would have to carry on information about "format" of the file (module, commonjs, json) and do the appropriate transforms based on implementation of translators from Node (essentially pass-through for ESM, and some handling for commonjs). I'm not keen on handling json files especially in light of #7623.

I'm not yet sure how to tackle this without wreaking havoc in current module loading logic to handle compat mode everywhere but it appears that rewrite of require to Rust is not necessary to support this feature.

@stale
Copy link

stale bot commented Jan 29, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

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

Successfully merging a pull request may close this issue.

2 participants