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

Better error messaging when using Eleventy’s --loader in Node #3441

Open
zachleat opened this issue Sep 9, 2024 · 3 comments
Open

Better error messaging when using Eleventy’s --loader in Node #3441

zachleat opened this issue Sep 9, 2024 · 3 comments
Labels
needs-discussion Please leave your opinion! This request is open for feedback from devs.

Comments

@zachleat
Copy link
Member

zachleat commented Sep 9, 2024

While this feature was originally intended for non-Node environments, we should add take additional care to throw an error when using Node with --loader?

Node can specify the default type using a type value in package.json or when using --experimental-default-type. https://nodejs.org/docs/latest/api/cli.html#--experimental-default-typetype

Primarily the problem here is that using npx @11ty/eleventy --loader=esm in Node (without specifying "type": "module" in package.json or --experimental-default-type=module) will still load the code via CommonJS (pre Node 22) and throw an error.

Secondarily, using npx @11ty/eleventy --loader=cjs in Node 22 and above doesn’t do much either due to Node’s improved handling of CommonJS/ESM.

Here’s where this value actually matters:

Proposals:

  1. Make the --loader override a no-op in Node.
  2. Should we use --experimental-default-type (when specified, taking precedence over a package.json type value)
    } catch (e) {

cc @vrugtehagel via #3377

@zachleat zachleat added the needs-discussion Please leave your opinion! This request is open for feedback from devs. label Sep 9, 2024
@vrugtehagel
Copy link
Contributor

vrugtehagel commented Sep 10, 2024

Yes, that error sounds problematic. We can either throw our own error (or warning) for using --loader=esm without "type": "module" but an alternative is to just throw an error or warning when using --loader with Node altogether. It doesn't really make a lot of sense to use the flag with Node anyway; the flag only exists because Eleventy is not able to correctly infer whether a project is ESM or CJS without a package.json. Node projects (at least ones that use Eleventy) always have a package.json and the "type" defined in the package.json should always match whatever style the author has chosen to use.

I will test this myself later, but it is also quite relevant to know whether or not Eleventy is able to correctly spider the dependencies in Node 22, where the "type" might not be set to match the file style. Note that the --loader flag still wouldn't really be necessary, since users can use the "type" field still instead of the --loader flag regardless of whether or not Node can run their project.

As for --experimental-default-type, probably it would be nice to support it either way, regardless of what we choose to do with the --loader flag in Node.

I probably won't have time to create a PR anytime soon, but will look at it in a few weeks if nobody else has already.

@zachleat
Copy link
Member Author

Appreciate you weighing in!

I would also add that spidering dependencies only matters during --serve and --watch so we might be able to further narrow the scope of this. Worth retesting how relevant that other code path still is (might be fixed in some Node versions now—not sure but worth testing)

@vrugtehagel
Copy link
Contributor

Just coming back to this with fresh eyes, and I think there's a bit of confusion going on here (at least from me, if not from both of us 😛).

I think the main source of confusion is the name --loader. The flag doesn't dictate how the code is loaded, but rather how the project is interpreted by the dependency spider. Setting --loader=esm doesn't make Node load your code as ESM, but it causes Eleventy's dependency crawler to interpret it as such by default (specifically around assuming what style .js files are using).

Primarily the problem here is that using npx @11ty/eleventy --loader=esm in Node (without specifying "type": "module" in package.json or --experimental-default-type=module) will still load the code via CommonJS (pre Node 22) and throw an error.

In a way, this is expected behavior. Either

  • The project is CommonJS, and that's why the "type" is unspecified and likewise the --experimental-default-type is ommitted. In this case, --loader=esm is telling Eleventy "pretend my .js files are ESM" and naturally that throws an error because the files are not ESM. Or,
  • The project is ESM, the "type" is unspecified and --experimental-default-type is omitted. This is a bit of a strange situation, at least pre-Node 22, because this causes an error regardless of whether or not the --loader option is specified; Node interprets the project as a CommonJS project and fails right when it encounters an import or other ESM syntax. This is not an issue that is resolved by the --loader flag, and this was never its purpose.

In Node 22 (at least, in the later sub-versions) this becomes a bit more interesting, because now Node understands both ESM and CommonJS even without specifying the --experimental-default-type. Eleventy can't; and even the --loader flag here doesn't help if the user has decided to mix the styles. For example, they could write their config file in ESM, but their data files in CommonJS. If they use .js extensions for everything, Eleventy is incapable of reliably creating a dependency tree. The --loader flag doesn't help here, because it simply changes how the files are interpreted by default, which fails on way or the other if the types are mixed. The one situation where --loader is useful in a Node evironment is when a user is using Node 22, does not specify the JS type in the package.json or otherwise, but uses ESM consistently. Then --loader=esm flicks Eleventy into the correct mode for the dependency spidering, whereas it would not otherwise be able to. Conversely, npx @11ty/eleventy --loader=cjs is hypothetically useful in the case where the user is using CommonJS consistently but has "type": "module" specified in their package.json (which is arguably a mistake, but this would work in Node 22 in theory).

I still think throwing a (more readable) error when using the --loader flag in Node, even Node 22, is a good idea; requiring users to correctly match their "type" in their package.json to their code style is, I think, a reasonable ask. In the case that Eleventy is running in another runtime, like Deno, and there is no package.json to infer the style from, then that is really the only case where the --loader flag is necessary and is only necessary if the runtime does not run CommonJS by default (which Deno doesn't). Note that with Deno 2 (even before that, but in a less streamlined way), there is much better compatibility with Node-style projects and then users can use a package.json to specify their dependencies rather than a deno.json. This would also allow them to specify the "type", even if the runtime itself does nothing with this information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Please leave your opinion! This request is open for feedback from devs.
Projects
None yet
Development

No branches or pull requests

2 participants