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
Merged
2 changes: 1 addition & 1 deletion src/lib/functions/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Contributor 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

}
} else {
this.buildFunctionAndWatchFiles(func, !isReload)
Expand Down
19 changes: 16 additions & 3 deletions src/lib/functions/runtimes/js/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import { Worker } from 'worker_threads'
import lambdaLocal from 'lambda-local'

import { BLOBS_CONTEXT_VARIABLE } from '../../../blobs/blobs.js'
import type NetlifyFunction from '../../netlify-function.js'

import detectNetlifyLambdaBuilder from './builders/netlify-lambda.js'
import detectZisiBuilder, { parseFunctionForMetadata } from './builders/zisi.js'
import { SECONDS_TO_MILLISECONDS } from './constants.js'
import { $TSFixMe } from '../../../../commands/types.js'

export const name = 'js'

Expand Down Expand Up @@ -99,11 +101,21 @@ export const invokeFunction = async ({ context, environment, event, func, timeou
})
}

// @ts-expect-error TS(7031) FIXME: Binding element 'context' implicitly has an 'any' ... Remove this comment to see the full error message
export const invokeFunctionDirectly = async ({ context, event, func, timeout }) => {
export const invokeFunctionDirectly = async ({
context,
event,
func,
timeout,
}: {
context: $TSFixMe
event: $TSFixMe
func: NetlifyFunction
timeout: number
}) => {
// If a function builder has defined a `buildPath` property, we use it.
// Otherwise, we'll invoke the function's main file.
const lambdaPath = func.buildData?.buildPath ?? func.mainFile
const { buildPath } = await func.getBuildData()
const lambdaPath = buildPath ?? func.mainFile
const result = await lambdaLocal.execute({
clientContext: JSON.stringify(context),
environment: {
Expand All @@ -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
Contributor 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
Contributor 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

})

return result
Expand Down
53 changes: 53 additions & 0 deletions tests/integration/commands/dev/functions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { test } from 'vitest'

import { withDevServer } from '../../utils/dev-server'
import { withSiteBuilder } from '../../utils/site-builder'

test('nodeModuleFormat: esm v1 functions should work', async (t) => {
await withSiteBuilder(t, async (builder) => {
await builder
.withNetlifyToml({
config: {
plugins: [{ package: './plugins/setup-functions' }],
},
})
.withBuildPlugin({
name: 'setup-functions',
plugin: {
onBuild: async () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires, n/global-require
const { mkdir, writeFile } = require('node:fs/promises')
await mkdir('.netlify/functions-internal', { recursive: true })
await writeFile(
'.netlify/functions-internal/server.json',
JSON.stringify({
config: {
nodeModuleFormat: 'esm',
},
version: 1,
}),
)

await writeFile(
'.netlify/functions-internal/server.mjs',
`
export async function handler(event, context) {
return {
statusCode: 200,
body: "This is an internal function.",
};
}
`,
)
},
},
})
.build()

await withDevServer({ cwd: builder.directory, serve: true }, async (server) => {
const response = await fetch(new URL('/.netlify/functions/server', server.url))
t.expect(await response.text()).toBe('This is an internal function.')
t.expect(response.status).toBe(200)
})
})
})
Loading