11ty / eleventy

A simpler site generator. Transforms a directory of templates (of varying types) into HTML.
https://www.11ty.dev/
MIT License
17.28k stars 495 forks source link

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

Open zachleat opened 2 months ago

zachleat commented 2 months ago

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) https://github.com/11ty/eleventy/blob/7ee002af21ac143a6f5b1ae9d4ff96f89a58077b/src/Eleventy.js#L1041

cc @vrugtehagel via #3377

vrugtehagel commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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

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.