Support for Node v14.x

[sodiray]

The changes from v12.x to v14.x are very small. All I really did was

• copy paste v12 dir
• update requires to imports
• update dynamic handler import in boostrap.js
• update test package.json to use module type
• add package.json in root of v12.x with module type

On my machine:
✅ build.sh completes and produces expected layer.zip
✅ test.sh completes and outputs expected log lines
🟨 publish.sh -- obviously I can't do this
🟨 check.sh -- obviously depends on ^^^

[mhart]

Awesome – thanks for giving this a shot – I was just gonna wait until official support TBH 😆

Might just be worth adding a top-level await statement to test/index.js to make sure that works – would be a nice-to-have

[sodiray]

Hey @mhart ! Loving the quick response! I would totally wait if I could but its one of those situations 😞 it was this or transpiling 🤮 (IMHO)

I added a top-level await ✅ works like a charm 🍀

[mhart]

Ok, so I've had a play – the main problem is this doesn't work out of the box with CommonJS modules – so it's not straightforward for people to just upgrade their existing Lambda functions.

I have no idea if something like this would work or not – I haven't played around enough with mixing ESM and CommonJS:

import { createRequire } from 'module'
global.require = createRequire(import.meta.url)

The other option would be to have an env var that toggles this behavior

[sodiray]

Based on my experience it will have to be one way or another during the build because of the type: "module" flag in the package.json. I'll do some r&d and see if there is a way to get node to handle it dynamically at runtime. You can probably guess I'm partial to ES6 modules as thats what I setup and of course use.

[mhart]

Basically, if we can get it to work with test/index.js from v12.x as well – that would be ideal.

I'm just not sure whether we should try for both out-of-the-box, or just ESM out-of-the-box with CommonJS support behind an env var, or CommonJS out-of-the-box and ESM behind an env var.

[sodiray]

based on Node docs

Node.js treats JavaScript code as CommonJS modules by default. Authors can tell Node.js to treat JavaScript code as ECMAScript modules via the .mjs file extension, the package.json "type" field, or the --input-type flag.

I'd say I set this up backwards. By default we should support CommonJS modules and allow for a flag to opt into ES6 modules. We might be able to handle an env var in the bootstrap and use the --input-type flag.

[mhart]

Yeah, I think that sounds right – I think it's better by default to assume traditional CommonJS modules

[mhart]

I haven't dived in yet, but for some reason this is noticeably slower to init than v12.x – which I suspect would also reflect in Lambda itself.

Any idea why that might be? Just the overhead of importing instead?

[mhart]

(that's why you see all the pings btw – because it takes so long to init – they ping every 100ms so usually you don't see them)

[sodiray]

I did add that 1 second sleep to the test file as the top-level await test. Not sure if thats where your noticing the performance?

[mhart]

Haha, lol – yeah, that'd be it – didn't even think of it

[sodiray]

Ok, r&d complete. Dynamic es6 modules | commonjs modules switching is not available in any workable way for our use case. I can think of some hacks but they involve eval or adding more scripts -- all bad things for performance and security. You may want to dig in a little yourself to validate this but my suggestion is we break the v14.x dir into two subdirectories:

-- v14.x
    |
    | -- modules
    |     | -- bootstrap.c
    |     | -- bootstrap.js
    |     | -- Dockerfile
    |     | -- package.json
    |     \ ...
    |
    \ -- commonjs
          | -- bootstrap.c
          | -- bootstrap.js
          | -- Dockerfile
          \ -- ...

then provide commonjs|modules specific layers.

[mhart]

Oh, we can't just do it behind a flag? Like, either import() or require() depending on an env var?

[sodiray]

After some more work I am happy to eat my words and say.... it works 🥳 A few updates in the last commit

• made two test directories
  • module
  • commonjs
• updated test.sh script to work with them as
  • ./test.sh module
  • ./test.sh commonjs
  • ./test.sh --> defaults to commonjs
• added a handler7 to the both test cases that explores some module|commonjs features just to be sure ✅ our implementation fully supports the differences -- not just the loader (require|import). See docs.
• updated bootstrap.js to handle NODE_MODULE_LOADER_TYPE environment variable
  • defaults to commonjs
  • validated to only allow 'commonjs' or 'module' to match node's defined usage
  • just as you said @mhart use either import() or require() based on the env var 🎉

[sodiray]

I published this in my aws account, based a lambda on it, and did a little testing ✅ NODE_MODULE_LOADER_TYPE env var is being handled well and importing/running code for both module types properly 👍

[brandonryan]

I know this issue is a bit outside the scope here, but I ran into it using your image and wanted to get another opinion.
I am using a lambda layer for all of my node_modules, because I have a lot of lambda functions that share a set of code.

Commonjs relies on the NODE_PATH env variable to add /opt/nodejs/node_modules to the require lookup. (relevant docs) With es modules, that has been disabled in node.
The suggested workaround from the docs is to use a symlink, but im not really sure how you would go about that in the lambda-runtime layer.

Thanks for the hard work putting together this runtime.

[sodiray]

Oh my page was open and didn't see you got it working! Yay!

[sodiray]

I don't love it... I don't like it... haha the warning on that --experimental-loader flag makes me so nervous. There is some sense of security against them changing the loader api because this layer is immutable in a sense -- once the layer.zip is created the encompassed node version can't change. Publishing layer for new 14.x versions would need to be done with caution because even a minor/patch update to node could cause a breaking change the loader api were using.

All that is probably already understood by everyone. I'm on the fence.

Another concern is performance. If I understand the way its written, it will always try to pull every required import from /opt/nodejs first and if that fails it will then try the normal import. I imagine this will have a noticeable impact on performance.

https://github.com/lambci/node-custom-lambda/pull/25/files#diff-fdb3033acc0a5a9767245a57a9cb21b76d794a6df53c9de560c0d4c6c51644caR1-R10

[brandonryan]

I agree with the node on performance. I think I did the priority backwards and it should try to load from the current path first, then /opt. That should mitigate any performance hit, but import errors might look a little weird. (unless i were to save the original error and throw it if any others aren't found)

I completely understand the hesitancy to use the --experimental-loader flag. It was just the only solution that i could find without some custom js require. My goal was to support es modules in the same way that commonjs modules were supported without any special code. This seems like all the more reason to add an ENV flag to turn that option on.

Will update with pull request in a bit.

[sodiray]

For sure. I'm actually a big fan of this solution aside from experimental state. If the api were stable I'd be thrilled. A few line custom loader is pretty slick.

I was thinking maybe we put this behind an opt in flag as well but I'm not sure it would really matter (that is If you switch the try order so it tries the default first). If you try to import a module and its not available your lambda is erroring out anyway -- whats another few ms to check /opt/nodejs in that situation?

Someone could also have some try { await import(...) } catch (err) { default case } like logic that would see a performance hit if we don't put the custom resolve behind a flag.

[brandonryan]

@rayepps made another pr. Changed the order of precedence and added the missing paths. Any modules resolved with the default resolution should take no performance hit now. I'm happy calling it complete now.

Note: I wanted to try to do some new async resolution but the defaultResolve function is synchronous so i couldn't

[mhart]

@brandonryan is the loader stuff only an issue if ESM is used? If so, we could just leave the experimental stuff up to the NODE_OPTIONS env var if ppl wanted to use it. ie, they can set NODE_OPTIONS env var on their Lambda to something like:

NODE_OPTIONS=--experimental-loader=/opt/esm-loader-hook.mjs

[mhart]

I feel like the best way to provide backwards compat is to default everything here to be in CommonJS – ie, bootstrap.js uses requires, etc. Then NODE_PATH will work as normal, as should any older Node.js code.

We can just use import() if the env var is set (would be good to see if there's any prior art for an env var name here? NODE_MODULE_LOADER_TYPE seems quite generic, so might have issues by clashing with something else – NODE_LAMBDA_MODULE_TYPE might be better?)

We could also explore whether it's worth auto-guessing based on file extension .mjs, or if there's a package.json at the top-level with "type": "module". Those methods might be cleaner in the end rather than relying on an env var. It's (usually) pretty easy for devs to just control the entry point of their handler or a package.json file.

[brandonryan]

@mhart the --experimental-loader=/opt/esm-loader-hook.mjs option is only used by node when es modules are being loaded, so no risk of commonjs issues there.
Relevant doc

When hooks are used they only apply to ES module loading and not to any CommonJS modules loaded.

Using NODE_OPTIONS for the loader flag would work, but I have two annoyances with it:

1. It would require the user of the lambda runtime knows where that file is
2. It would not adhere to the /opt dependency convention as described in the aws docs by default. The user of the runtime has to do an extra step to make work what works in every other runtime by default.

That being said, im not the maintainer of the repo, so use your own discretion.

As a side note, I just now thought it might be a good idea to read NODE_PATH then use the paths from there instead of using a hard coded list in the loader. I can make that change if you want me to.

Currently everything does default to commonjs.
I really like your idea about auto detecting whether the source in task root is mjs or reading the package json. I think thats the way to go.

[mhart]

My issues with adding the experimental flag by default for ESM modules is that 1) it's experimental and 2) it could add time to the loader – for every import statement. If you've got a Lambda doing hundreds of imports, that could have quite an impact.

It would not adhere to the /opt dependency convention

What do you mean by this?

[brandonryan]

Im good with that reasoning. Since it tries to load from the default loader first it shouldn't be that big of a performance hit, but I understand the reservation.

I meant that most official lambda runtimes are configured to support importing packages from /opt as the default, and not supporting that out-of-the-box would be breaking convention.

[mhart]

@brandonryan yeah, that's a fair point. Would be nice if everything "just worked". Maybe the impact wouldn't be that bad. We could make it an opt-out deal perhaps.

[brandonryan]

I'll make some more updates soon with your suggestions. Working my day job right now so it'll be a few hours.

[brandonryan]

@rayepps
Alright! Made a pull request here for the auto detection sodiray#3
I also fixed some other random bugs i ran into when i was testing.

Side note: I'm going to make another issue for this, but for some reason errors aren't reporting in the lambda console correctly.

[brandonryan]

@mhart pull request updated. I didnt add the c code to add an opt out flag because im not experienced enough in c to confidently do that.

[Kivol]

Hi, I wonder if there's any update on this PR? It would be great if lambda 14.x is supported!

[Kivol]

I've suggested some improvements regarding this PR:
sodiray#4

[matthewhaywardmsm]

Hi @mhart, would it be possible to get this PR sorted? I've got a local test stack which depends on the lambci nodejs12 docker image but we want to update to nodejs14 and this is the only thing holding it back.

[mhart]

@matthewhaywardmsm can you be more specific about how this layer affects your test stack?

[matthewhaywardmsm]

We need to be able to run multiple lambdas locally (as part of a local implementation of AWS Cognito which has multiple lambdas), and the way we've done it is to use the lambci/lambda:nodejs12.x docker image as a base image for each of them. We want to update to nodejs14, but currently there is no version 14 available of lambda:nodejs12.x base image. This layer appears to be a custom prerequisite to building the lambda:nodejs12.x image, so I thought this would need updating in order to make nodejs14 buildable in lambci/lambda.

[mhart]

@matthewhaywardmsm I think you might be looking at the wrong repos. You probably want lambci/docker-lambda#329

[matthewhaywardmsm]

Thanks @mhart, I ended up using amazon/aws-lambda-nodejs:14 as a base image instead.

[trivikr]

Thank you @rayepps for posting this PR, and @mhart for creating this project.
I was able to build and validate Node.js 16.x custom runtime layer by changing the configuration as follows:

export LAYER_NAME=nodejs16
export NODE_VERSION=16.20.0