Modules: Upstream PR for utility method for detecting ES module syntax

[GeoffreyBooth]

I’m thinking of opening a PR against core for https://github.com/nodejs/ecmascript-modules/pull/69. Does anyone have any feedback for me before I do so?

Here’s what I plan to say about the PR:

---

This PR adds a utility method to detect whether input JavaScript source code contains ES module syntax:

const { containsModuleSyntax } = require('module');

containsModuleSyntax('import { shuffle } from "lodash"'); // true
containsModuleSyntax('console.log(process.version)'); // false

containsModuleSyntax('import "./file.mjs"'); // true
containsModuleSyntax('import("./file.mjs")'); // false

The use case for this is any utility that currently uses vm.Script’s runInThisContext and might also want to support ESM input source code, and therefore would need to know when to use vm.Script versus the ES module vm.SourceTextModule (and has no definitive signal from the user as to the intended source code type, and where the risk of evaluating in the wrong module system is acceptable for the use case). Two such utilities that I’m familiar with are Babel and CoffeeScript, which each take arbitrary source code as user input (like --eval) and use vm.runInThisContext for evaluation. I would also imagine that this method would be useful to some loaders or testing frameworks.

[ljharb]

I’d expect usage of with to force it to be false as well, but otherwise lgtm.

[mathiasbynens]

And HTML comments.

[GeoffreyBooth]

My method detects whether source code contains syntax that's permitted only in the Module parse goal. You're describing the inverse, detecting syntax that's only permitted in the Script parse goal. In theory such a method could serve as a companion to this one, but it would have very limited usefulness as Script-only syntax is generally all obsolete and almost never appears anymore in real world code.

[devsnek]

they're saying you should force the result to false if html comments and/or with statements are present.

[ljharb]

This is a generic utility method, it should be generically applicable.

[GeoffreyBooth]

It should also be fast, and checking for things that are unlikely to be present slows it down with no benefit.

[devsnek]

correctness > speed? we're discussing the evaluation of random code after all...

[GeoffreyBooth]

There’s nothing incorrect about containsModuleSyntax. It returns true if Module-only syntax is present, false otherwise. That’s all it aims to do and all it needs to do for the use cases I’ve presented.

You can propose a companion containsScriptSyntax that does the inverse, returning true when it finds syntax that is allowed only in Script. The only use case I can think of for such a method is if someone wants to essentially create a pragma to force Script-mode only, like <!-- use Script -->, because that’s the only reason I can think of that Script-only syntax might be present in source code. That can be its own PR.

Another detection we discussed was looking for references to the CommonJS globals require etc. That would be far more useful than searching for Script-only syntax, as pretty much no real-world code _has_ any Script-only syntax. But again, that can be its own PR.

[jkrems]

I’d expect usage of with to force it to be false as well, but otherwise lgtm.

I assume it would only not already be false if the text contains import/export syntax. And in those cases the choice is "script with syntax error or module with syntax error". Or am I missing something? If that's the case, returning true or false is equally "correct" I would argue.