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

support cjs and mjs file extensions #1847

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

llimllib
Copy link

@llimllib llimllib commented Nov 25, 2024

I was surprised to find that these default node file extensions weren't supported by default, so I added them

This is particularly helpful so that you can write data loader scripts with top-level await and with ESM import syntax, by saving them with the .mjs file extension

@Fil
Copy link
Contributor

Fil commented Nov 25, 2024

Is it not possible to make the default .js extension smarter instead? I'm not talking about using bun (though that works perfectly IMHO), but by adding relevant options such as --experimental-top-level-await?)

@llimllib
Copy link
Author

I haven’t tested (will do later) but I would guess that would break with cjs-style files?

@mbostock
Copy link
Member

@Fil What do you mean by “make the .js extension smarter”? Do you mean that even though we register .js, Framework would automatically look for .cjs and .mjs as well? I dunno, feels to me like this approach is simpler and doesn’t require Framework to re-implement Node’s resolution logic.

@Fil
Copy link
Contributor

Fil commented Nov 26, 2024

We can add default support for more extensions. My suggestion however was not about that, but rather to explore if we could pass more options to node on .js files, so that we could support import and top-level await in those files. But maybe that would break some existing code that uses require? I don't know. It'd be great to have concrete use cases (tests) and a bit of documentation.

I'm also experimenting with deno:
interpreters: {".ts": ["deno", "--allow-net", "--allow-read", "--allow-ffi", "--allow-env"]},

@llimllib
Copy link
Author

llimllib commented Nov 26, 2024

--experimental-top-level-await is accepted as an argument, but doesn't seem to do anything on a regular js file

`--experimental-top-level-await` test
$ node --experimental-top-level-await test.js
(node:94342) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/private/tmp/prasebbref/test.js:1
const res = await fetch(
            ^^^^^

SyntaxError: await is only valid in async functions and the top level bodies of modules
    at wrapSafe (node:internal/modules/cjs/loader:1383:18)
    at Module._compile (node:internal/modules/cjs/loader:1412:20)
    at Module._extensions..js (node:internal/modules/cjs/loader:1551:10)
    at Module.load (node:internal/modules/cjs/loader:1282:32)
    at Module._load (node:internal/modules/cjs/loader:1098:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:215:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:158:5)
    at node:internal/main/run_main_module:30:49

Node.js v22.4.0

$ cat test.js
   1   │ const res = await fetch(
   2   │   "https://www.basketball-reference.com/leagues/NBA_2025.html",
   3   │ );
   4   │ const data = await res.text();
   5   │ const table = data.match(/<table.*id="advanced-team".*?<\/table>/)[0];
   6   │ const rows = [...table.matchAll(/<tr.*?<\/tr>/g)];
   7   │ const tabledata = rows
   8   │   .map((row) => {
   9   │     return Object.fromEntries(
  10   │       [
  11   │         ...row[0].matchAll(
  12   │           /<(?:td|th).*?data-stat="(.*?)".*?>(.*?)<\/(?:td|th)>/g,
  13   │         ),
  14   │       ].map((res) => [res[1], res[2]]),
  15   │     );
  16   │   })
  17   │   .filter((t) => t.team?.match(/\/teams\//));
  18   │ // clean up the data a bit
  19   │ tabledata.forEach((t) => {
  20   │   t.team = t.team.match(/\/teams\/(.*?)\//)[1];
  21   │   delete t.DUMMY;
  22   │ });
  23   │ console.log(JSON.stringify(tabledata));

The old documentation suggests that this option may be obsolete, as mjs files now support top-level await by default, so I don't think that option does anything

Here is the documentation on when node will choose to go into ESM mode versus commonjs mode.

It's a very unfortunate situation, but it seems to me that letting people opt in to one or the other explicitly via their choice of file extensions would be useful in framework

@Fil
Copy link
Contributor

Fil commented Nov 26, 2024

Yes, I'm convinced. Thanks for the detailed explanation!

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.

3 participants