Support ES Modules

[taeold]

Updates loader.js's getUserFunction to detect and load user code packaged as an ES module.

To determine whether user code is ES Module or CommonJS, we use the following "algorithm" as described in NodeJS package documentation:

1. A module with .mjs extension is an ES module.
2. A module with .clj extension is CommonJS.
3. A module with .js extensions where nearest package.json's with "type": "module" is an ES Module
4. Otherwise, it is CommonJS.

Note that dynamic import function used to load ES modules is only available on NodeJS v13.2.0 and up (proof). The code throws an error if the version of NodeJS on the running process is less than v13.2.0. (In practice, this means that GCF users must use nodejs14 if they want to deploy a function packaged as an ES module).

Fixes #233

[grant]

Thanks! Looks good, I'll test when I get back in office. I added a few comments and suggestions.

[grant]

Is there a library that already implements this logic? It might not be unique to this module.

Maybe we can use this?

https://www.npmjs.com/package/is-file-esm

[taeold]

newbie question: Do we prefer to use an external package when possible or is there any reason to limit external deps in this repo?

The list of deps for this repo was pretty slim, and I had small amount of hesitation adding new ones. The package you linked seems to be well tested, but I can imagine going the other route and beefing up the unit tests for this function rather than adding a dep.

[grant]

If an external package seems useful, not abandoned, and genetic for the problem, then use it. I like to remove code that isn't unique to this library / i.e. could be used in other packages as package dependencies. We do keep dependencies small in general.

[taeold]

I took a closer look at https://www.npmjs.com/package/is-file-esm 's source code. I'm being very nitpicky, but it seems to me that it has bit more complex implementation than what I have here. To be fair, it does more, but we don't need those extra functiond ality.

I lean towards keeping couple of lines and keep this logic to ourselves, but that's just me coming from go-world where I'm used to doing things like this. If you think we should opt-in to use that package, I'll be happy to revise.

[taeold]

Whoops. Discovered that is-file-esm makes use of queueMicroStack in https://github.com/davidmarkclements/is-file-esm/blob/master/index.js#L9 which is only available on NodeJS 11 and up.

I think functions framework needs to work for nodejs10 for the foreseeable future. Does this make using the library a deal breaker?

[grant]

Oh, hmmm.

Let's keep the lines here then.

---

Aside, Node 12 should be latest LTS but we need to update min engine to that. But I'm not sure if that will take a bit.

[grant]

Thanks a bunch!

[grant]

Main test

1. Globally install forked FF (with ESM support): clone fork, npm run build && npm i -g ., functions-framework --target helloWorld
2. Create a ESM function.

export const helloWorld = (req, res) => {
  res.send('Hello, World!');
};

3. Run functions-framework --target helloWorld

Works as expected.

Other testing

• Tested with Node 10:
  • If using a module in a unsupported version, user functions will give a nice error:

  Cannot load ES Module on Node.js v10.22.1. Please upgrade to Node.js v13.2.0 and up.
  Could not load the function, shutting down.
  

  • Using without a module works as expected
• Tested with Node 14:
  • ESM works as expected (export const helloWorld)
  • Non-ESM without type: module works as expected

[matthewrobertson]

Overall, this looks good. Thanks for doing this!

[grant]

@taeold It seems like this PR needs some logic for windows for ESM support.

Maybe we need to add something like a file:// prefix with getFunctionModulePath?

Error I see:

Detailed stack trace: Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'

We could also consider not supporting ESM for Windows for now.

[taeold]

Related nodejs/node#31710

[taeold]

@grant The fix to support Windows seems pretty straightforward.

[grant]

@matthewrobertson I re-approved running the last checks. Over to you.

[bcoe]

@taeold awesome work, I'm very excited for this contribution, from the perspective of someone who helps maintain our client libraries.

[matthewrobertson]

LGTM, thank you!

[aryehb]

@taeold I don't understand how this is supposed to work for .mjs files. require.resolve() throws an error for .mjs files, so getFunctionModulePath() will always return null and the script will exit.

There's import.meta.resolve(), but it's still experimental and behind a flag, it requires that you already know the extension, and it can't be used in CommonJS modules.

The only way I could figure out to make .mjs files work is to use the following to manually check for the existence of codeLocation/index.mjs:

import { promises: fs } from 'fs';

// -----------------
// In getUserFunction()

const _path = path.resolve(codeLocation, './index.mjs')
await fs.stat(_path);
functionModulePath = _path;

[taeold]

@aryehb Can you clarify? Are saying that the functions-framework does not currently work with .mjs files? If so, can you help us track the issue by filing a bug with repro-steps?

As for .mjs files and require.resolve(), the code is relying on the user code to specify main attribute of package.json, e.g.:

$ ls
index.mjs package.json
$ cat pacakge.json
{ 
  "name": "functions",
  "main": "index.mjs",
...
$ node
> require.resolve(".");
'/path/to/index.mjs'

I

[aryehb]

I see what you mean. I was able to get it to work using main. This works because the project root is passed as codeLocation to require.resolve() in getFunctionModulePath(), which resolves the path to the .mjs file using the main field, even though it's an ESM.

My issue was that I was using the --source=dist/ CLI flag with no main field. This caused the path that is passed as codeLocation to require.resolve() in getFunctionModulePath() to be the path to dist/, which causes it to throw an error Cannot find module 'dist/'.

I'm not sure why there is a --source flag if the main field works. Regardless, I think it should be documented that the main field can also be used, and that using --source without main won't work for .mjs files.