Skip to content
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

Merged
merged 9 commits into from
May 15, 2024

Conversation

@Skn0tt Skn0tt self-assigned this May 6, 2024
@Skn0tt Skn0tt requested a review from a team as a code owner May 6, 2024 10:56
@Skn0tt Skn0tt requested a review from lukasholzer May 6, 2024 10:56
Copy link

github-actions bot commented May 6, 2024

📊 Benchmark results

Comparing with 79732ad

  • Dependency count: 1,341 (no change)
  • Package size: 311 MB ⬆️ 0.00% increase vs. 79732ad
  • Number of ts-expect-error directives: 992 ⬇️ 0.10% decrease vs. 79732ad

@@ -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))
Copy link
Member Author

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'),
Copy link
Member

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?

Copy link
Member Author

@Skn0tt Skn0tt May 7, 2024

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?

Copy link
Member

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?

Copy link
Member Author

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:

Screenshot 2024-05-07 at 13 19 48

@Skn0tt Skn0tt requested a review from eduardoboucas May 13, 2024 12:30
@Skn0tt Skn0tt enabled auto-merge (squash) May 15, 2024 09:12
@Skn0tt Skn0tt merged commit 334152c into main May 15, 2024
48 checks passed
@Skn0tt Skn0tt deleted the support-mjs-functions branch May 15, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants