doc: add spec for contains module syntax

[GeoffreyBooth]

This PR updates the ESM resolution spec to correct the language about how we do the “contains module syntax” check.

@nodejs/loaders @guybedford

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[aduh95]

Suggested change

	> * `import` statement (static only, _not_ dynamic `import()`)
	> * `export` statement
	> * `import.meta`
	> * `await` at the top level
	> * A lexical redeclaration of any of the CommonJS wrapper variables
	> (`require`, `exports`, `module`, `__filename`, `__dirname`) at the top
	> level
	> * A static `import` statement.
	> * An `export` statement.
	> * Usage of the `import.meta` meta property.
	> * An `await` expression at the top level.
	> * A lexical redeclaration of any of the CommonJS wrapper variables
	> (`require`, `exports`, `module`, `__filename`, `__dirname`) at the top
	> level.

[GeoffreyBooth]

I think it's important to clarify that this is only for import statements and not expressions.

[guybedford]

Note that [[HasTLA]] is already an ECMAScript module field - https://tc39.es/ecma262/#sec-cyclic-module-records.

[guybedford]

I worry about tying this detection design to the exact implementation, as opposed to defining what it is checking. Do you think it would be equivalent to claim the detection is the following:

1. If the source contains await at the top-level, or static import or export syntax, or contains import.meta, return true.
2. If the source contains a top-level lexical redeclaration (const, let, or class) of any of the CommonJS wrapper variables require, exports, module, __filename, __dirname, then return true.
3. Return false.

What I prefer about this definition is that we are fully tying it to the exact check rules, as opposed to tying it to all the minutia of JS parser error behaviour.

[GeoffreyBooth]

Do you think it would be equivalent to claim the detection is the following:

Yes, I think that would be equivalent. What steps would you replace with that text?

I also think it should say “static import or export statements (not dynamic import() expressions)” because I think others who might look at this spec to reimplement it, say in bundlers, might not catch the nuance that they should exclude import() unless we spell it out.

[aduh95]

others who might look at this spec to reimplement it, say in bundlers, might not catch the nuance that they should exclude import() unless we spell it out

I think implementers of all people are the least likely to miss that nuance.

[guybedford]

Yes, I think that would be equivalent. What steps would you replace with that text?

If that is the case, I'd suggest we replace the existing steps with the steps in #52059 (comment) to remove dependence on ECMA-262 parsing error behaviours (which may also change over time).

[guybedford]

So basically, to flip it around to not depend on a CJS parse in future here, and instead ensure the ESM path is the performance optimized one:

> Parse _source_ as an ECMAScript module.
> If the parse is successful,
  > If _source_ contains top-level await, static `import` or `export` syntax or `import.meta`, return *true*.
  > If the source contains a top-level lexical redeclaration (`const`, `let`, or `class`) of any of the CommonJS wrapper variables `require`, `exports`, `module`, `__filename`, `__dirname`, then return *true*.
> Return *false*.

Then we can effectively leave "error reporting" itself as undefined for tooling and bundlers, since every environment can handle their own errors, so we don't need to strictly define that. By allowing that to be a grey area, this should allow us to switch between CJS-first and ESM-first parsing detections I believe? But I may also have missed something so let me know your thoughts.

[guybedford]

Perhaps DETECT_MODULE_SYNTAX to capture that this is the "detection path".

[GeoffreyBooth]

flip it around to not depend on a CJS parse

Well our implementation has to parse as cjs first in order to avoid performance regression. If we define the spec as parsing as esm first then we'll be out of compliance, because ambiguous code that can parse as either would then need to run in strict mode per the spec.

[guybedford]

If we define the spec as parsing as esm first then we'll be out of compliance, because ambiguous code that can parse as either would then need to run in strict mode per the spec.

The compliance says nothing about error handling - only when to treat it as ESM or CJS. Ambiguous code always executes as CJS, regardless of parsing. So where is the non-compliance here?

[GeoffreyBooth]

Ambiguous code always executes as CJS

I guess you’re right; I was thinking of code like console.log(this) but I guess in your version it still runs as CJS. So I guess your version provides the same outcome as our implementation, though it’s the inverse of the actual implementation. What’s the benefit of spec’ing it in this way as opposed to how we’ve actually implemented it?

[guybedford]

What’s the benefit of spec’ing it in this way as opposed to how we’ve actually implemented it?

We instead frame things towards ESM being the optimized parse case, where CJS is an accommodation for the current parsing approach, so that we can optimize ESM as the primary parse in future. We could even document that the reason for the CJS variable handling is to allow for the CJS parsing preference today but that it is designed to work both ways?

[GeoffreyBooth]

@guybedford I implemented your suggestion, please let me know what you think.

[nodejs-github-bot]

Landed in f1949ac