Enhancement: Support .mjs & .cjs extensions

[phated]

Supersedes #50 & #59 - Please see them for additional context

We currently have support for ESModules using the .esm.js extension and the esm module to transpile them on the fly. However, node 13 (and the upcoming 14) support the .mjs and .cjs extensions. I've run into a bunch of troubles reviewing, interpreting, and testing those features, but I'd like to support them.

I'd love some help here @ulrichb @cedx or anyone else with experience.

One other note: I believe this will be a breaking change because it seems the .cjs extension can't be used outside a .mjs instance.

[Haringat]

@phated This will definitely be a breaking change as you cannot require() a .mjs file. However, you can use import() inside both .cjs and .mjs files (in both cases it returns a promise).

Thus you would have to use import() notation instead of require() to dynamically resolve the gulpfile.

That would require to break backwards compatibility because e.g. in Node.js 12 import is already a reserved word and if node stumbles upon it while parsing your code it will just throw an error telling you to enable experimental modules support.

[ExE-Boss]

One other note: I believe this will be a breaking change because it seems the .cjs extension can't be used outside a .mjs instance.

That’s incorrect, as .cjs can be used anywhere a .js file can be used currently, which both the docs and the source code confirm.

[phated]

@ExE-Boss you must have missed this in the node docs:

Node.js will treat the following as CommonJS when passed to node as the initial input, or when referenced by import statements within ES module code:

• Files ending in .cjs

Gulp uses require to load files, so cjs won't "just work".

[ExE-Boss]

Gulp uses require to load files, so cjs won't "just work".

That is factually incorrect, as require('./some-file.cjs') works just fine, and both the docs and the source code confirm this:

Regardless of the value of the "type" field, .mjs files are always treated as ES modules and .cjs files are always treated as CommonJS.

This is because require(…) treats any extension it doesn’t know about as .js.

[phated]

Interpret is used to register require extensions and doesn't allow for node's fallback behavior. I also believe node didn't used to have that fallback behavior, but I'd need to check when I'm not on mobile.

[ExE-Boss]

Node has had the fallback to .js behaviour for a long time due to extensionless files, which are supported for command line purposes on UNIX.

[Haringat]

Interpret is used to register require extensions and doesn't allow for node's fallback behavior.

That is wrong because even with interpret you are still calling node's require() on the file and if at that point the extension has not been defined node's require() will use its fallback functionality.

[yannbf]

I have recently seen an issue on storybook at storybookjs/storybook#9854
where a dev wanted to use .mjs for his files but it didn't work. As storybook relies on interpret, indeed it wouldn't work given that there's no support for mjs so far.

However, by adding the following in interpret extensions array:

'.mjs': [
    {
      module: '@babel/register',
      register: function(hook) {
        hook({
          extensions: '.mjs',
          rootMode: 'upward-optional',
          ignore: [ignoreNonBabelAndNodeModules],
        });
      },
    },
  ],

And also .mjs in the jsVariantExtensions array, the files were loaded successfully.

I might be ignorant about this but shouldn't that already help?

[Haringat]

@yannbf That attempt still relies on a transpiler (in this case babel) to work. We can already do that using the esm package. What this issue is about, however, is leveraging Node's native support for esm modules so that no transpiler is necessary.

[yannbf]

Hey @Haringat got it! Thanks for explaining.

[snoack]

As for backwards compatibility, at least in Node.js 10.19.0 and 12.16.2, import(...) just returns a rejected promise (if the --experimental-modules flag is unused). So how about some code like this to enable loading of ES modules in a backwards-compatible way?

function loadGulpFile(filename) {
  try {
    return Promise.resolve(require(filename));
  }
  catch (e) {
    return import(filename).catch(() => { throw e; });
  }
}

[phated]

@snoack whoa! great information, I think that is definitely something we need to investigate. Want to help out?

[snoack]

I already had a quick look at gulp, gulp-cli and interpret and tried to figure out how the Gulpfile is loaded but I'm a bit lost there. Do you have any pointers?

[phated]

@snoack yep, that's right here: https://github.com/gulpjs/gulp-cli/blob/master/index.js#L204-L206

[snoack]

Thanks, but it seems that a version-specific wrapper is imported there, while the Gulpfile itself is imported from that wrapper (e.g. here). Do I understand that correctly? Would it be a problem to make that wrapper asynchronous (required to import ES modules in there)?

Also where is env.configPath coming from? I assume it defaults to gulpfile.js? Where is that default defined?

BTW, what is the target in terms of backwards compatibility? I was under the impression that Node.js 10 (the oldest still supported LTS version) is as far back as anyone cares about backwards compatibility. But looking at your CI, I'm impressed to see tests passing on versions as old as Node.js 0.10. Do I see that right? It's not necessarily a deal breaker, but I might have to look some more into backwards compatibility if you plan to keep supporting all of those versions of Node.js.

[phated]

@snoack supporting this will need to require 10+, which will require a major bump in gulp-cli. People will be able to install the major version globally for backwards-compat with gulp, but it will also ship with gulp 5.

You are correct that gulp is loaded there and it is determined by Liftoff. If interpret defines .mjs as null, it should find the filename.

[ExE-Boss]

This is more correct, as it ensures that the correct error is reported:

function loadGulpFile(filename) {
	try {
		return Promise.resolve(require(filename));
	}
	catch (errRequire) {
		return import(filename).catch(errImport => {
			if (errRequire.code === "ERR_REQUIRE_ESM") {
				throw errImport;
			}
			throw errRequire;
		});
	}
}

[snoack]

@phated I had a go. It's still WIP, but I could already use some feedback. BTW, it seems I found a way to keep things backwards-compatible all the way back to Node.js 0.10.

[phated]

BTW, it seems I found a way to keep things backwards-compatible all the way back to Node.js 0.10.

😍 Oh my, this would make you my favorite person! I won't have time to review tonight, but hoping to check it out soon.

[snoack]

I finished up my pull request (with test case and everything). Feel free to review.

[snoack]

@phated, I skimmed through afe7c29. But I am not familiar enough with interpret to fully understand the change, in particular what you are doing there with stubs, and how you treat Node.js 10 special (in the tests at least).

I just want to clarify on Node.js >=10.15.3, I'd expect gulp --experimemtal-modules to pickup gulpfile.mjs from the current directory and load it as a native ES module. On Node.js ^12.17.0 and >=13.2.0 the --experimemtal-modules flag is no longer necessary, but otherwise nothing changed.

[phated]

@snoack if the --experimental-modules flag makes the require hook throw, things should still work. When setting a require.extensions hook to null, any require with that extension is passed to the standard require handler, which causes that to throw (in my local testing).

[snoack]

Yeah, that is what is confusing me. Why not simply setting ".mjs": null (leaving it up to gulp-cli to deal with it)? Mind that strictly speaking you cannot even tell from the file extension alone whether a script is ESM or CJS.

If some project has a gulpfile.mjs, I think it is a very safe assumption that it requires a Gulp and Node.js version that supports it (if necessary called with --experimental-modules).

Anyway, as long as gulp --experimental-modules will pickup my gulpfile.mjs on Node.js 10, I'm pleased. :)

[phated]

@snoack yeah, unfortunately the reasoning is buried under levels of indirection. The liftoff module uses the rechoir module to lookup and register a loader for the extension if it doesn't exist in require.extensions and the node core team never added an entry for .mjs in that hash, so rechoir was crashing when specifying the value as null. Alas, the stub is needed.

[phated]

Anyway, as long as gulp --experimental-modules will pickup my gulpfile.mjs on Node.js 10, I'm pleased. :)

Testing this locally now and it is working. Needed to add 1 more update to your changes and should be good.

[phated]

@snoack sure, but for the sake of fun, open a node REPL on node 10.21 like node --experimental-modules and type require.extensions - they added the hook in this version of node and could have it in newer versions too. You can also .toString() it and see it just throws the error you check for in gulp-cli.

[snoack]

Testing this locally now and it is working. Needed to add 1 more update to your changes and should be good.

Awesome, well appreciated! (Curious about that change though.)