--type=auto module type detection algorithm

[GeoffreyBooth]

Per nodejs/modules#263 (comment)

Feedback invited! @bmeck @devsnek @guybedford @jdalton @jkrems @ljharb @SMotaal

I’ve invited the folks that have been involved in discussions related to --type=auto or this proposal in general. Once it seems okay to you all, I’ll open an issue on the main modules repo to invite broader feedback.

[devsnek]

this ain't a reference

[GeoffreyBooth]

What is it?

[jkrems]

Maybe:

• A return statement in the top scope.

[bmeck]

If we want to be very specific:

• A return statement in the top VariableEnvironment of the source text

[bmeck]

I'm assuming this applied to returns within things like block statements.

[GeoffreyBooth]

So really it's a return statement in the top function scope?

[bmeck]

Not a function scope necessarily. So no, not a function scope.

[GeoffreyBooth]

It's not? Doesn't the CommonJS wrapper create a function scope that encompasses the entire file? So a return anywhere in there would exit that wrapper function scope, unless the return were inside a child function scope?

[bmeck]

But that is circular, you must treat it as CJS for you to check if it is CJS in that case. Nothing is preventing Module (or god forbid Script) from adding early returns except going through committee. Same for await. You should state the heuristics based upon the overlap, which would just be the VariableEnvironment; which is the Module environment for Modules, the Global environment for Script, and the Function environment for CJS. Only stating it on the function environment doesn't make sense since this is just a heuristic, we cannot assert that it is a function environment in order to check if it is a function environment.

[GeoffreyBooth]

Is it really that common to use return in the top scope (or whatever I should call it) without the CommonJS globals? I'm wondering if we need to bother with the checks beyond looking for use of those globals (including checking that the globals aren't defined by user code).

If we limit the CommonJS check to just the globals, then the explanation for this feature is straightforward: import/export is run as ESM, use of CommonJS globals is run as CommonJS, anything else errors. The simplicity of that might be worth a few false negatives (my "anything else" group) if these other checks are unlikely to grow our haul of detected CommonJS significantly.

[devsnek]

@ljharb we're currently only allowing this algo to succeed if -r and _third_party_main.js and such aren't used.

[ljharb]

@devsnek i'm not sure what that's in reference to.

[devsnek]

@ljharb none of those variables could exist as globals

[ljharb]

@devsnek yes, but that doesn't mean that a reference to them proves it's ESM - it just means that such code couldn't complete.

[GeoffreyBooth]

@ljharb I’m asking for collaboration, not just criticism, if you don’t mind. I appreciate you pointing out issues in this document, but if you don’t mind can you please couple those points with suggestions for how the document can resolve those issues? And if you can’t think of any solutions, please tell us what you considered and why none of your ideas worked.

[ljharb]

@GeoffreyBooth i apologize that it's come off as criticism; my suggestion is "remove any heuristic that depends on identifiers". Only syntax can reliably disambiguate.

[GeoffreyBooth]

So with regards to detecting CommonJS globals, yes, the following could be ESM:

require('./file.js');

And of course it would immediately throw, since we’re defining this file as an initial entry point and so we therefore know that require isn’t defined here. So basically while it could be ESM, it would be useless ESM, and therefore it’s a safe assumption that it’s CommonJS.

So maybe my language at the top about being able to “unambiguously” detect CommonJS is too sweeping, and needs to be dialed back a bit. I think users would still prefer we look for references to the CommonJS globals as our primary mode of detecting CommonJS, rather than excluding them because they aren’t absolutely positively 100% proof of CommonJS. Or looked at another way, I would say that references to CommonJS globals is proof beyond a reasonable doubt, and that’s good enough for the purposes of this algorithm.

Could-run-as-ESM code could be written that could reference such globals and not throw, via code like

if (require) {
  // . . .
}

but likewise, I think this is safe to assume that this code should be run in CommonJS mode. There’s a big difference between code that references the CommonJS globals and code that’s truly ambiguous. If you look at the syntax/keyword differences in the list, I think they’re too obscure for us to use as CommonJS signifiers. Too many obviously CommonJS files would lack such signifiers of Script mode, for example any CommonJS file that has 'use strict' (I think) and therefore far too many CommonJS files would be classified as ambiguous.

[jdalton]

I'm not a fan of trying to detect CJS either. The approach by @zenparsing here might be promising. It tries to compile (not run) the entry module as CJS and if it errors it tries as ESM.

[ljharb]

That's also not sufficient; there exists modules that can parse and run successfully in both CJS and ESM, but have different semantics; it's not safe to evaluate these as either without an explicit signal.

[ljharb]

@GeoffreyBooth i don't think it would be a good idea for node to ever "guess" - if it's not certain, it should throw, and relying on identifiers is not a certainty. If someone has a file that -a can't support, then their solution is simple - either add syntax, add a file extension, or provide whatever command line flag that can serve as an explicit signal. "auto" is a fine convenience, but it shouldn't ever be a footgun.

[zenparsing]

@ljharb I can appreciate how that seems like it might be an issue. Can you provide a concrete example of how that might be a problem in practice?

[ljharb]

@zenparsing i can come up with a contrived example that wouldn't be very convincing, but either way I think node needs to draw a line based on what's possible, not merely based on what's likely. The parsing goals are ambiguous, and as such, it is simply not possible in every case to disambiguate them without an additional signal.

[GeoffreyBooth]

The parsing goals are ambiguous, and as such, it is simply not possible in every case to disambiguate them without an additional signal.

I think that’s the argument for why -a should never be the default (non-flagged) behavior, but not why referencing the CommonJS globals isn’t good enough to consider a file likely enough to be CommonJS to run as CommonJS. The user is opting into this behavior, we deserve to give them a feature that’s useful.

[ljharb]

We're not userland - we don't have the luxury of being fuzzy/loose. "Likely" doesn't matter; node itself should only parse files as X when it knows them to be X, for certain.

[jdalton]

FWIW I figure I should probably mention the approach I take with auto detection in the esm loader. Under "auto" I look for early parse goal indicators like extension,.mjs, and parse goal pragmas. In the absence of those I parse as Module, and if it errors I parse as Script. I then inspect the AST and only treat the code as Module if it contains a handful of ESM only syntax (static import/export, etc.). There's other optimizations involved but that's the gist.

[GeoffreyBooth]

I think a reference to a CommonJS global, where that global isn’t defined in the file and the file is an initial entry point, is certain that any real-world code is CommonJS. Otherwise the code is written to try to trick the detector; I just don’t see what realistic useful program would exist otherwise. @ljharb I understand you may disagree, but unless we have a better algorithm I consider this one to be good enough, even for Node core, for an opt-in feature. If I’m outvoted down the line then so be it.

We have a corpus we can test against. My research repo has 941 projects from NPM that define "module" in their package.json files. By definition all those files should be ESM, though many of them might be invalid ESM (using CommonJS globals along with import/export, because Babel allows that). The packages all also generally have a "main" with their CommonJS entry point. We could run all those files through an implementation of our algorithm and see if we get any false positives.

[ljharb]

Using a corpus of projects that are highly likely to be ESM-only is one thing; I'd be interested to see the majority of npm, which is highly likely to be CJS-only.

[evanplaice]

Speaking from the other side of the aisle (ie FrontEnd), the majority of the existing libs are IIFEs with AMD/UMD/CJS tacked on.

Unfortunately, if you're determining type by matching on a 'require' statement it's going encounter a lot of false positives. At least, until the ecosystem has some time to transition.

[ljharb]

I can also do:

global.require = () => {}; // or `global['re' + 'quire']`

require();

which would match that heuristic as CJS, since there's no declaration, but could easily be both ESM or CJS.

[GeoffreyBooth]

The proposal includes checking for require “without a declaration for that variable.” That should catch the UMD etc cases and Jordan's realistic one.

Again, I'm looking for collaboration not criticism. If all you have to offer are problems and not at least your thought process for a solution, please keep your objection to yourself. I'm not looking to defend this document from a firing squad, I'm asking for help improving it.

[ljharb]

My example does not contain a declaration for that variable.

@GeoffreyBooth these are suggestions to improve it - removing things that aren't viable helps improve the document, and is absolutely collaboration, not criticism.

[GeoffreyBooth]

@ljharb a suggestion for improving it would be to clarify that I should've written an assignment for the global variable. That's what I'm asking for.

[ljharb]

Right, but I'm saying that relying on identifiers is not viable, so my suggestion continues to be to remove all such heuristics.

[GeoffreyBooth]

Unless you have an alternative that still works, specifier detection is the best heuristic I know of. Removing it without a viable replacement makes this feature worthless. So you can suggest another solution, otherwise your point is noted.

[ljharb]

To clarify further, global['re' + 'quire'] = represents dynamic creation of a global variable, which can not be detected without evaluation, so any identifier is simply not a reliable option. If that makes the feature worthless, then the feature is unfortunately worthless. If I can think of another solution, I'll definitely comment with it.

[GeoffreyBooth]

I don't consider any of the examples that would foil detection to be realistic. I think it's worth adding a section to the document listing the edge cases that would be false positives and negatives, to show that those edge cases aren't likely to appear in real user code. As an opt-in feature, I don't think it needs perfect accuracy.

So if you'd like to contribute you can write such a section, and help catch as many edge cases as possible. We need to catch common realistic cases like AMD, which I think is doable. If there's a big class of realistic code that's a false positive I'll reconsider, but for now I think a feature that works the vast majority of the time is worth building and acceptable as an opt-in feature.

[evanplaice]

Scratch my earlier comment. I think it's safe to say, multi-mode modules that support CJS should always be detected as CJS. Since, 'module.exports' and 'export default' don't work together in the same file anyway.

Besides, it's not difficult to convert an IIFE so it can be imported as a default ESM.

Converting existing libs that heavily rely entirely on CJS are another story. Those will take much longer to update.

[GeoffreyBooth]

I’ve updated the document per the comments above. Feedback is welcome, but if your feedback is to point out a problem with the document I respectfully request that you suggest an improvement to the document to address your objection.

I added a paragraph to discuss @ljharb’s point about references to the CommonJS globals being allowable ESM. I still think detecting the globals, including checking that they’re not getting defined, is a good enough heuristic for CommonJS until someone can show me some practical real-world code where that fails. I’m thinking it should exclude as ambiguous most frontend cases like Browserify or AMD/UMD, since those patterns either define global require or define a local one that they use. I can’t think of any non-theoretical cases where a non-CommonJS file would reference CommonJS globals, aside from detecting whether or not a script is running in a CommonJS environment (and in that case, the script is therefore written to potentially run in such an environment, like jQuery’s NPM package).

I also removed the detection for arguments, this and return at the top level(-ish) from CommonJS, as I don’t think those happen terribly often and without those checks, the algorithm is both faster and simple enough to explain to users:

• import/export, it runs as ESM.
• References to CommonJS globals, without defining those globals, it runs as CommonJS.
• Otherwise error.

[GeoffreyBooth]

Thanks @ljharb, I made the corrections you suggested. That’s the kind of feedback I was looking for 👍

With regards to eval, you don’t mean to use it as a signifier of CommonJS (since it appears to be valid in Module goal as well) but rather as a signal that maybe some funny stuff is going on here and therefore we shouldn’t think that we can assume anything about the file? As in, someone could do eval('global.require = ...') and therefore fool the detector, so if we see eval at all we should just error?

I think if someone is trying to deceive the algorithm they’re going to succeed, like in your earlier global['req' + 'uire'] = example. But I guess what is to be gained by such tactics? The only way to get code to run as ESM is to include import or export, full stop, so there’s no chicanery that can cause code to run as ESM that would also run as CommonJS or Script. All that can be gained via sneaky tactics is to get some code to run as CommonJS that might otherwise error as ambiguous, or vice versa. If you want to trigger code to run as CommonJS, you can just reference one of the CommonJS globals. If you want to trigger it to error as ambiguous, you can assign one of the globals e.g. global.require = require. I guess doing the latter has a purpose if someone wants to guard their code against being runnable via --type=auto, but there are many more direct ways of doing so (such as using .mjs or .cjs).

The purpose of checking for redefining the CommonJS globals is to filter out legitimate use cases like UMD or Browserify, though even they probably aren’t defining require as global, so maybe that check isn’t necessary. But likewise, I’m not sure what purpose would be served by a user trying to game the algorithm toward or away from CommonJS execution or erroring, so I don’t know why we would try to prevent that (even if we could).

[ljharb]

What i mean is, if it’s not syntactically ESM, and it’s thus a CJS candidate, I’m suggesting using “use of eval” as a disqualifier, to eliminate some edge cases that could make “CJS” the incorrect goal.

[GeoffreyBooth]

I’m suggesting using “use of eval” as a disqualifier, to eliminate some edge cases that could make “CJS” the incorrect goal.

What edge cases does it disqualify? I can’t think of any other than someone intentionally trying to trick the detector, which I don’t think we should be worrying about since they have so many other ways to do so besides eval (and I also don’t see why anyone would bother).

I also like the simplicity of defining autodetected CommonJS files as files that use the CommonJS globals, full stop, without needing to explain a bunch of extra items we’re detecting; unless those extra detections add something significant.

[GeoffreyBooth]

@jdalton and I were discussing this and he suggested making the algorithm even simpler:

0. For initial entry points that are ambiguous (.js file outside of any package scope or inside a package scope with no "type" field; or source input via --eval or STDIN):
1. Parse the source.
2. If the source has an import or export statement, execute it as ESM.
3. Else execute it as CommonJS.

And that’s it. Ambiguous files would run as CommonJS, just like how node ambiguous.js (with no package.json with a "type" field) would run as CommonJS already today.

• Is there the potential for running non-ESM code as ESM? Yes.
• Is that likelihood high? No: there really shouldn’t be too many ESM initial entry points that don’t import or export anything.
• But it’s nonzero? Yes—that’s why this algorithm is so simple that it can be explained in a sentence, and why it’s opt-in, and we’re trusting users. As per the previous point, the likelihood of real-world ESM initial entry point code lacking import or export statements should be so low that this wouldn’t be a footgun.
• Does this approach have precedent? Yes, this is how other tools like Babel and @jdalton’s esm identify ESM files (I’m sure I’m oversimplifying here, but he can explain in more detail). They don’t try to affirmatively detect CommonJS.
• But what about ambiguous files? We’re just defining them as being executed as CommonJS and that’s that, just like what the node command does. If anything that’s what users are already familiar with; an error that a file is ambiguous would cause some serious head-scratching, as rare as such an error is likely to be.

I’m not saying I’m entirely convinced that this is a better way to go, but he makes a strong argument. This algorithm entirely removes the worries about the detector being fooled or gamed; either the source contains import/export or it doesn’t. The simplicity offers some measure of reliability, and fits more neatly in with the “be straightforward” approach of Node when it comes to things like wanting an .mjs extension etc. Thoughts?

[ljharb]

Is there the potential for running non-ESM code as ESM?

Do you mean vice versa?

I think this simpler algorithm works pretty well for extensionless files; i do not think it should work for package.json-less .js files - we should not silently override an explicit signal, and “.js” is an explicit signal that it’s not ESM (lacking additional explicit signals, like a package.json)

[GeoffreyBooth]

Do you mean vice versa?

Yes, thanks.

I don't think a file being .js without a package scope is by itself explicit of it being intended as CommonJS. In my research I found thousands of ESM .js files. That's the use case this flag is intended for.

[ljharb]

How many of those were written to be node entry points, without babel, or a flagged experimental node feature, in mind?

[GeoffreyBooth]

How many of those were written to be node entry points, without babel, or a flagged experimental node feature, in mind?

And how many of those ESM entry points intended for use with Babel might also be intended to be evaluated directly in runtimes that support ESM? There’s no way for the author to tell us. We simply can’t assume anything about a loose .js file. It may have been intended for Babel, for the browser, for Deno, for anything. If the user hasn’t made it explicit, it’s an unknown.

I’m not really interested in debating when to disallow this feature. There’s an argument to be made that --type should always take top precedence, as the user is making their intentions abundantly clear and the user takes priority over the author (since the user can always edit the files to be run, so it’s not like the author has any protection). That can be a later debate in the larger group.

As for this algorithm itself, as opposed to what it can apply to, does anyone have any other notes?

[GeoffreyBooth]

I’m also realizing, import and export keywords need to exist at the top level, as in, they cannot be inside a block like an if; and they’re keywords, so they’re always written as import(space) or export(space); so couldn’t we search for them via regex? Like (line boundary)import(space)? Something like:

if (/^(?:im|ex)port\s/m.test(sourceCode)) esmDetected();

This saves us from having to work with any parsers, which feels like a win. @SMotaal is there a better regex than this one? Is there any problem with using regex for this?

[devsnek]

you can't use regex to search for anything in js because (as one example) I can start any line in js with a string, and regex can't parse a js string.

[SMotaal]

@devsnek I think we visited this one before in #203 and while my demo is eagerly begging for a serious refactor, it proves that RegExp based "semi-contextual" parsing is a viable thing that like any other will have it's own limitations but can also comes with unique (ie different, no claim if it is better or worst) optimization potential.

I have since found few cases where I continued to improve the safety from a sense of curiosity in slowly extending this when it seems viably suited. For instance, thanks to @guybedford's comment in es-modules-shim, I found and fixed a failure where expressions like ? a / b : c /d tokenized incorrectly, but not any more.

I am not plugging this in, I continue my work because I personally see potential which qualify at least to merit not claiming that it is not possible ❤️ please.

For fun you can try: https://www.smotaal.io/markup/packages/@smotaal/tokenizer/examples/browser/ and refer to the DEMO section of this README to use this mostly RegExp parser to highlight sources from unpkg or plain https (cors)

[devsnek]

@SMotaal you can use regex for individual matches when building tokens (the tokenizer holds all the context that regex doesn't have) but you can't use regex over a whole source text, as Geoffrey asks above

[GeoffreyBooth]

Okay, forget I asked about regex, that’s an implementation detail.

Anyone have a rough idea on how this could be implemented? Would it be parsed with Acorn or is there some way to use the V8 parser? @zenparsing?

[devsnek]

@GeoffreyBooth i would want to use a full parser. i think it's important for our method for getting the kind of module something to also validate that the source text is valid. we shouldn't return a valid output for an invalid input.

[GeoffreyBooth]

@GeoffreyBooth i would want to use a full parser. i think it’s important for our method for getting the kind of module something to also validate that the source text is valid. we shouldn’t return a valid output for an invalid input.

Yes, that’s what my last comment said? Are you saying that Acorn or the V8 parser aren’t full parsers? Is there something else you would use?

I updated the PR to match our discussions, I think this is ready to merge in now.

[SMotaal]

Absolutely, a RegExp tokenizer can be easier to conceive relative to @ljharb's matchAll proposal, where you have this generator internally updating the lastIndex on the goal's matcher pattern (ie RegExp instance) based on the currentContext, generating a token from it's exec and updating the currentContext from the token's nextContext… etc.

So the tokenizer mechanics here are the missing contextual component of what is normally isolated context-free regex patterns.

In all cases parsing scans every character, in some methods parsing will incur "additional" code execution costs for every character, but ultimately every lexeme (token of meaningful characters) should incur code execution costs regardless of method for contextual validity to hold. So prior to template strings, when all masquerading literals where either flat or homogeneously structured, you could do a regexp|strings|comments|… matcher and then you are left with code tokens, but that is no longer the case.

As for invalid code, @devsnek you put this very nicely, any operator used to "translate" — which is a term I saw used in --experimental-modules and resonated to me as really clear runtime-optimized counterpart to "transpile" or "compile" — should be impartial to invalid code, including SyntaxError, this is the job of the runtime's own parser regardless of any translation implementation requirements. This is very very critical to avoid message massaging errors into obscurity as we've seen with certain popular things complaining about the wrong thing when runtimes are happy to call them the right thing and that thing was what you intended in the first place. 👍

Edit: Just for clarity, I am symbolically considering translation in the context of --type=auto as the operation executed on a sourceText record with a {type = "auto", body = <readable>} as fields which returns with the same record updated to {type = "module" | "script"} (or whatever is decided elsewhere) leaving the body in its initial consumable state.

[SMotaal]

@GeoffreyBooth my conclusion from going over the final update here is that RegExp or AST is a detail for the following parsing scenario (roughly):

LET token = first()

WHILE ( token ):

  IF ( [ImportKeyword, ExportKeyword].includes(token.type) ) THEN:
    RETURN "module"

  ELSE IF ( isBlock(token) ) THEN:
    WHILE ( inBlock(token = next()) ):  // import.meta not considered here
      CONTINUE                          // inner

  ELSE THEN:
    token = next()

  CONTINUE                              // outer

RETURN "script"                         // or whatever not module is here

Some of the considerations here are:

1. Return should prevent further parsing to avoid unnecessary parsing costs.

   It should also reset the state of the body to be consumed if it is a stream.

2. Executing the wrong thing is separate from the thing being "module" or not.

3. Presence of import.meta inside function body is an implementation decision.

   This can look like this:

   …
   
   ELSE IF ( isBlock(token) ) THEN:
   
     token = next()
   
     WHILE ( inBlock(token) ):          // import.… is always "module"
       IF ( token.type === ImportKeyword ) THEN:
         IF ( (token = next()) AND (token.type === PeriodPunctuator) ) THEN:
           RETURN "module"
   
       ELSE THEN:
         token = next()
   
       CONTINUE                         // inner
   
     CONTINUE                           // outer inBlock === false
   
   …