-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: bug around v1 functions with nodeModuleFormat: "esm"
#6557
Conversation
@@ -434,7 +434,7 @@ export class FunctionsRegistry { | |||
|
|||
func.mainFile = v2EntryPointPath | |||
} catch { | |||
func.mainFile = join(unzippedDirectory, `${func.name}.js`) | |||
func.mainFile = join(unzippedDirectory, basename(manifestEntry.mainFile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem here was that esm files end in .mjs
, not .js
. By using the basename from the manifest, we'll have the right extension
@@ -118,6 +130,7 @@ export const invokeFunctionDirectly = async ({ context, event, func, timeout }) | |||
lambdaPath, | |||
timeoutMs: timeout * SECONDS_TO_MILLISECONDS, | |||
verboseLevel: 3, | |||
esm: lambdaPath.endsWith('.mjs'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already running ESM functions locally today, so I'm not sure I follow why we're adding this now. What does it do exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lambda-local complained about ESM files not being requireable, so I had to add this. Under the hood, it makes lambda-local dynamic-import the file, instead of requiring it. I'd guess that right now, we're compiling those ESM functions to CJS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We respect the module format specified by the file extension and the module
property. So we're keeping ESM files as ESM, at least in production. I'd be surprised if this wasn't the case locally. The CLI itself is ESM, so by importing lambda-local
as ESM it should be able to import both ESM and CJS files without a dynamic import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But lambda-local is a CJS module, and if we don't set the esm
property then it will always require
the file at lambdaPath
. See here: https://github.com/ashiina/lambda-local/blob/8914e6804533450fa68c56fe6c34858b645735d0/src/lambdalocal.ts#L337-L344
I'm not sure why this hasn't been a problem before, but if I remove this line locally I get:
Resolves https://linear.app/netlify/issue/COM-434/internal-v1-functions-with-esm-nodemoduleformat-are-broken-in-local