feat: support individual function paths in zipFunctions
#6481
Conversation
|
This pull request adds or modifies JavaScript ( |
|
Some follow-up thoughts from our Slack thread:
On the SDK side, we can also do a major release to relieve us from having to preserve any old behavior on any of this. (That would be my vote--I think this new behavior is an improvement in all ways. On the SDK side, we can document how to set the bundler to For these reasons: Wondering aloud if it'd make sense to write the Other reason I have in mind: IIRC the SDK doesn't currently support authoring non-JavaScript/TypeScript functions, but it'd be nice to remove this this restriction to reduce the number of SDK-specific quirks developers need to keep in mind. (I suppose we could detect the function language and write a per-language entrypoint, too.) I guess we'd only need to inject this shim if the user is writing a JavaScript/TS file, though... 🤔 Anyway--not at all attached to the mechanism by which we signal node module lookup paths to the bundler, just throwing out some thoughts/use cases/etc. Anywho, a stub entrypoint written by the SDK to the internal functions/edge functions directories might look something like this: import * as exp from "{{path to function source in .netlify/plugins/extension_buildhooks directory";
export * from "{{.netlify_directory}}/.plugins/{{extension_buildhooks}}/index.js";
export const config = {
nodeBundler: "esbuild",
scopedToFunctionDirectory: true,
...exp.config,
}; |
|
One additional use case to consider that's particularly relevant to these approaches: Whatever we go with should support function transformation prior to packaging to e.g. change function configuration. I think it'd be more powerful if we could transform raw source, but it'd be an acceptable tradeoff to limit this API to changing function |
|
This pull request adds or modifies JavaScript ( |
eduardoboucas
changed the title
scopedToFunctionDirectory option to ZISIzipFunctions
|
This pull request adds or modifies JavaScript ( |
|
This pull request adds or modifies JavaScript ( |
|
This pull request adds or modifies JavaScript ( |
|
This pull request adds or modifies JavaScript ( |
|
@ndhoule The PR has changed substantially since you last commented, but I'll still address some of your points:
There is nothing here that is specific to V2 functions, but in any case I think it should be up to the SDK to decide what artefacts to generate (i.e. whether to use the old or the new way).
No longer applicable, as we no longer use a stub.
I'm not sure if I'm misunderstanding your suggestion, but doing this will cause us to inline all the modules, which is the problematic behaviour we're trying to move away from?
As a small note, this wouldn't work because the in-source configuration block needs to be fully static, because we extract it using static analysis.
I'd need to understand a bit better where and when you'd like to do these transformations — e.g. prior to packaging the extension or the final functions? The latter should still be possible, because the build plugin is in control of where the function lands, so it could point to a file that has been rewritten by the plugin? |
|
This pull request adds or modifies JavaScript ( |
|
Great, I'm glad shifting away from the stub approach preempts so many of these issues.
To be clear: I'm with you, I'd rather not use esbuild at all. Injected functions are currently built using esbuild, though, so there's a chance that someone is relying on esbuild-specific quirks, so we should be mindful of that. That said: I think this is something we can manage on the SDK side, either via a code change or a major SDK release.
I think the short of it is: Given your updated approach, I think we'll be able to expose a transform API that serves the need. More info below: Here's one of the use cases for transforming functions prior to injection. Specifically, this is a use case where we need to modify an injected function's routing patterns (set in the function's exported (We don't have to discuss the particulars of that transform API and the pros/cons of each approach here. Mainly I bring it up as a potential design constraint because it informs the ergonomics of the API. E.g. a post-build API would likely have issues around, do we need to surgically modify the generated |
| @@ -64,10 +64,19 @@ const zipFunction: ZipFunction = async function ({ | |||
| return { config, path: destPath, entryFilename: '' } | |||
| } | |||
|
|
|||
| // If the function is inside the plugins modules path, we need to treat that | |||
| // directory as the base path, not as an extra directory used for module | |||
| // resolution. So we unset `pluginsModulesPath` for this function. | |||
There was a problem hiding this comment.
This comment is super valuable, and I think it'd be helpful to expand on the "why" in this comment. (My understanding: It's because we expect functions in build plugins to be self-contained/include all their own dependencies; if we don't unset this value, we may resolve missing dependencies using the project's dependencies.)
I don't know if you can make that... uh, less wordy...
|
This pull request adds or modifies JavaScript ( |
|
|
||
| return filenames.map((name) => join(srcPath, name)) | ||
| } catch (error) { | ||
| // We could move the `stat` call up and use its result to decide whether to |
There was a problem hiding this comment.
I thought about adding a feature flag, but this is simpler and I think it makes the rollout safe enough.
|
This pull request adds or modifies JavaScript ( |
Summary
With this PR, the
zipFunctionsmethod accepts both paths to functions directories (existing behaviour, unaltered) as well as paths to individual function files.Additionally, zip-it-and-ship-it will check whether the function's entry file is inside the plugins modules path (i.e.
.netlify/plugins), which it's already aware of, and if so set the base path to that directory, to ensure that module resolution doesn't escape that boundary.