Fixes and improvements for current AWS-provided Node runtimes - #155
ScottMorse wants to merge 20 commits into
Conversation
brandonbothell
left a comment
There was a problem hiding this comment.
Shouldn't you rename index.js to index.mjs?
|
@brandonbothell Since |
|
Thank you for the info, I believe that since it's recommended in the README as a possible installation method, we should aim to support the copy+paste method with as minimal user-confusion as possible.
|
…runtime versions)
|
@brandonbothell Makes sense! I went ahead and performed the update:
I also just noticed that the README lists Node 18, 20 and 22 as the current Lambda runtimes, so I updated this to 20, 22 and 24, and the |
| "main": "index.mjs", | ||
| "scripts": { | ||
| "check-coverage": "c8 report --reporter=lcov && c8 check-coverage --statements 100", | ||
| "lint": "npx eslint", |
There was a problem hiding this comment.
Should this be updated to npx eslint@^9.18.0 (or we can update dependencies?)
There was a problem hiding this comment.
Oh, npx is actually redundant here. package.json scripts have node_modules/.bin in their PATH, which is why you can invoke dependencies' executables there directly.
npx will similarly use node_modules/.bin if it's present from the cwd. If npx is called and the command isn't found in node_modules/.bin, that's when it installs the latest version of a package dynamically (or uses its cache) to run it.
So in this instance, npx eslint is no different than plain eslint within "scripts".
The "eslint" package is installed as a peer dependency of the other ESLint-related packages, which supplies the eslint command and may differ in version from @eslint/js.
I am seeing now that when I delete my package-lock.json and run npm install, it downloads eslint at version 10.2.0, so running npx eslint --version in the repo then reports v10.2.0 to me, the same as if I run npm run lint -- --version regardless of whether npx eslint or eslint is used in the package.json script.
However, version 10 doesn't seem to be working with the plugins being used, so it's probably worth locking down the version of "eslint" to use to that it's not up to chance anymore. I can make a simple change for all this.
There was a problem hiding this comment.
Yeah, this makes sense as why I had to pin the npx command @^9.18.0. Thanks again for the useful info, your suggestion sounds good!
There was a problem hiding this comment.
No problem! I author a package that has a CLI for Bun, and package.json, node_modules/.bin, and bunx (the npx equivalent) work essentially the same way, which is the only reason I actually understand how this stuff works.
I went ahead and made the package.json changes for this, just setting the "eslint" version and simplifying the script to eslint.
|
That makes a lot of sense! In the back of my mind, while I was replying
earlier, I wondered if the original author did use `npx` for a reason. Is
it possible that if a user has a global installation of `eslint` in their
path above NPM's that isn't compatible with the project's dependencies,
that just having `eslint` in the script could cause issues?
…On Fri, Apr 17, 2026 at 4:28 PM Scott Morse ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In package.json
<#155?email_source=notifications&email_token=AIRI62QCINGQLBIJN22UNPT4WKHWBA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJTGE4TANJTGEYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJL3QOJPXEZLWNFSXOX3DNRUWG2Y#discussion_r3103080398>
:
> @@ -3,7 +3,7 @@
"version": "6.0.0",
"description": "Serverless email forwarding using AWS Lambda and SES",
"type": "module",
- "main": "index.js",
+ "main": "index.mjs",
"scripts": {
"check-coverage": "c8 report --reporter=lcov && c8 check-coverage --statements 100",
"lint": "npx eslint",
No problem! I author a package that has a CLI for Bun, and package.json,
node_modules/.bin, and bunx (the npx equivalent) work essentially the
same way, which is the only reason I actually understand how this stuff
works.
I went ahead and made the package.json changes for this, just setting the
"eslint" version and simplifying the script to eslint.
—
Reply to this email directly, view it on GitHub
<#155?email_source=notifications&email_token=AIRI62VZLQY3IHLHWDAOWC34WKHWBA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJTGE4TANJTGEYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJPLQOJPXEZLWNFSXOX3ON52GSZTJMNQXI2LPNZZV6Y3MNFRWW#discussion_r3103080398>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIRI62TCQ5Z4ARW2SJUMRPD4WKHWBAVCNFSM6AAAAACXOTVKLKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DCMZRHEYDKMZRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
|
@brandonbothell I don't think that's the case. I have a feeling it's probably just that the author used |
|
Thanks for the explanation. I think this PR is good to go when/if the
maintainer is ready to merge it then.
…On Fri, Apr 17, 2026 at 5:16 PM Scott Morse ***@***.***> wrote:
*ScottMorse* left a comment (arithmetric/aws-lambda-ses-forwarder#155)
<#155?email_source=notifications&email_token=AIRI62RAREUKCFDJK7OV5IT4WKNKLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRXGEZTIMBZGE4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4271340918>
@brandonbothell <https://github.com/brandonbothell> I don't think that's
the case. npx eslint still defaults to node_modules/.bin/eslint
regardless of whether a global install of eslint is present. When you use npm
i -g eslint, you do have the eslint executable on your user's PATH, but
the node_modules/.bin path still takes precedence over that for both npx
or the plain eslint script.
I have a feeling it's probably just that the author used npx eslint to
run it before adding the script in package.json and didn't realize or
forgot that it's unnecessary.
—
Reply to this email directly, view it on GitHub
<#155?email_source=notifications&email_token=AIRI62RAREUKCFDJK7OV5IT4WKNKLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRXGEZTIMBZGE4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4271340918>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIRI62REXJJDADSRQS4EYML4WKNKLAVCNFSM6AAAAACXOTVKLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DENZRGM2DAOJRHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Issues
Fixes (#154)
Main issues
index.jsinto the Lambda console for a quick setup, Lambda now defaults to usingindex.mjs, so the CJS syntax breaks unless one knows to update the handler filename.Related/secondary issues
.lengthto detect a callback vs. async handler, so theoverridesmust be split from the main async handler function in addition to dropping thecallbackparam.nycfor testsindex.jsis inaccurate from being hard-coded from a previous versionSolutions
index.jsfunctions to be async arrow functions as the fix for Node 24createHandlerto splitoverridesparam out ofhandlerhandler.lengthto detect callback vs. async handler use, so it will still throw a deprecation error in Node 24 without this, even if correctly using the async pattern.createHandlerindex.jsfunctionscreateHandlerfor overrideshandler.jstest to use explicitly rejectedPromise, instead of causing unobviousTypeErrorvianext().index.js,example/, andtests/imports and exports"type": "module"topackage.jsonsourceTypeto"module"in ESLint config"use strict"since this is the default for ESMc8for tests instead ofnycpackage.jsonscripts to approximate oldnycusageconsole.loginindex.jsto reflect current package version (would need to be updated again on version bump)Notes