This does not work with dynamic or conditional module imports

[ehmicky]

Some Node modules do this:

try {
  require('moduleName')
} catch (error) {}

Or this:

if (condition) {
  require('moduleName')
}

Or this:

eval(`require('moduleName')`)

Or this:

require(getModuleName())

A common example is node-fetch which conditionally require encoding. encoding is only used with the textConverted method, which throws if it's missing.

Another example is @nestjs/graphql which uses apollo-server-core but does not declare it in its production dependencies.

The reason why some modules might want to do this and not use optionalDependencies is to allow users to opt-in to installing specific modules.

However this creates the following two issues with zip-it-and-ship-it. When the conditional require() is performed:

1. Directly from a function file, zip-it-and-ship-it always bundles the dependency. Users should have a way to exclude such modules in the archive if they want to.
2. From a node module, zip-it-and-ship-it never bundles the dependency. This is because we find nested dependencies by only checking the package.json dependencies, peerDependencies and optionalDependencies keys (as opposed to look for require() statements). Users should have a way to include such modules in the archive if they want to.

The current workaround are (for the points above):

1. User should npm install the dependency
2. User should npm install the dependency and add a noop require() call in its function file code.

However this feels hacky, so we should think of a better solution.

[DavidWells]

Almost all function bundling solutions have the concept of includes & excludes options

This covered the missing pieces when trying to parse the AST or use recursive package.json files to includes the exact files needed for deployment.

example

We most likely need a higher level option for users to explicitly include/exclude files from the packaging step during the build

[ehmicky]

An additional use case: some libraries create their source files at runtime or during postinstall. For example, Prisma generates its client and outputs it under node_modules/.prisma.

This is a little different from the cases above because we crawl Node modules differently. Instead of using require() statements, we use the files field from package.json. This does not work with dynamically generated source files.

[Rich-Harris]

Bumping this issue, as it was the first problem I encountered when trying to use Netlify functions. It seems to me that the correct behaviour here would be to ignore modules that can't be resolved — if they fail at runtime, so be it (though as @ehmicky describes this is often used to detect the presence of optional dependencies, meaning 'failure' is generally intentional in these cases).

Short of that, a warning could be printed to help track down the cause of those runtime errors.

With the current behaviour, you have to jump through some very complex hoops to get widely-used packages like node-fetch working with functions, even once you've diagnosed the problem. The end result is that functions are essentially unusable for apps that depend on those packages.

[witcradg]

Possibly related to issue #205.

[kentcdodds]

Remix will need this (or they'll have to do some extra fanciness during the build). I've put together a simple example repo (without remix) that works locally, but not in production:

• https://github.com/kentcdodds/netlify-dynamic-require
• https://condescending-bassi-78ec13.netlify.app/.netlify/functions/index

Here's what it looks like locally:

[ehmicky]

Hi @kentcdodds,

What's not currently supported are require() statements that are not top-level (e.g. inside an if or a function) or whose argument is not a string literal. Your example repository should work as the require() is not dynamic (instead, it returns a function, statically). I have reproduced it and the endpoint works in production.

After cloning your repository, it looks your problem is different (see build logs). In order for Netlify Functions to work properly, you currently need a build command, otherwise those functions won't be bundled by zip-it-and-ship-it (except locally, as you noticed). We are actually currently fixing that bug. In the meantime, using # as your build command should fix this problem. If you've been deploying manually or through the CLI, the situation might also be different. In all cases, I'd suggest opening a separate issue for this, as this seems unrelated to dynamic requires.

[kentcdodds]

Ah, thanks for clearing that up @ehmicky. So I've updated the project to make it an actual dynamic require and added a build command. Once dynamic requires are supported, then I'll get it updated to serve as an example :) Thanks!

[ehmicky]

Thanks for the additional information @kentcdodds!
Do you have some additional insights into the requirements of Remix for this feature?
I am especially wondering about which type of dynamic require() is needed? Among:

• conditional require(): inside an if block
• require() inside a try/catch block
• lazy require(): called inside a function
• dynamic argument: require() which argument is not a string constant

[kentcdodds]

I'm not sure actually. All I know is the files need to be in a specific structure and require-able by that structure at runtime

[ryanflorence]

@ehmicky We just need an include option to specify which files to also deploy to the function so that dynamic requires work. It's not about "conditional requires", it's non-static requires.

For example, other serverless functions like vercel have an option to indicate other files that are needed that you won't find by static analysis of requires:

{
  "includeFiles": ["remix.config.js", "build/**/*.js"]
}

[ryanflorence]

Little more background, when the remix request handler gets a request, it matches against the routes and then dynamically requires the "route loaders" (functions that fetch data server side for the view). We're considering changing our build step to turn all of that into static requires in the entry of the function, but at the moment it's dynamic.

An include option would let us use netlify right away.

[reimertz]

@ehmicky We just need an include option to specify which files to also deploy to the function so that dynamic requires work. It's not about "conditional requires", it's non-static requires.

For example, other serverless functions like vercel have an option to indicate other files that are needed that you won't find by static analysis of requires:

{
  "includeFiles": ["remix.config.js", "build/**/*.js"]
}

Agree with this!

Seems like such a low hanging fruit enabling the ability to bundle things such as content editable with Netlify CMS.

[ehmicky]

Thanks a lot everyone for your feedback! We are looking into solving this problem and are considering different options.

Whichever solution is chosen would need to allow including/excluding files both:

• In the Netlify Function itself: for example, if a functions/example.js is doing a dynamic require
• In the Netlify Function's dependencies: for example, if a Function is using a library like Remix which would do a dynamic require

This raises the following questions:

• Where does the include/exclude list go: in netlify.toml, in plugin inputs, in the site package.json, in the Function file (e.g. as code comments), in a separate configuration file?
• When using a library with dynamic requires, who should specify this list: the dependency's maintainers, the dependency's users, or both?

@calavera mentioned #225 as a possible solution for this, providing it does solve those two questions.

[kentcdodds]

I vote the include/exclude should go in netlify.toml. However I also vote that both a dep's maintainer as well as the dep's users should be able to specify include/exclude. For that to work, it probably makes sense to allow this to be configured in the package.json (as well?)

Redacted due to @ryanflorence's great points below

[ehmicky]

Please note that when it comes to Functions' dependencies, zip-it-and-ship-it automatically bundles any files that's been published to npm (with some minor exceptions like lock files or source maps) (see logic here). So, as a Node module maintainer, Netlify Functions will bundle any Node module's files providing they are published to npm. This has some shortcomings though: too many files are included (no tree shaking) and it does not work with files generated at runtime or at postinstall. I am wondering whether this might be sufficient for Remix though @kentcdodds?

The situation is different with the Netlify Functions files themselves (as opposed to the Netlify Functions dependencies). For those, we parse the Function's JavaScript code, look for top-level require() statements and only include those dependencies (tree shaking).

[kentcdodds]

I think that's sufficient for remix. AFAIK, remix only dynamically requires user code.

[Offirmo]

Regarding issue 1), couldn't we solve it by not requiring the module. If it's present in node_modules, add it. If it's not available, don't fail.

[ryanflorence]

@ehmicky

Where does the include/exclude list go: in netlify.toml

Yes. Seems like the only place to do it, from your docs:

The netlify.toml file is your configuration file on how Netlify will build and deploy your site

When using a library with dynamic requires, who should specify this list: the dependency's maintainers, the dependency's users, or both?

Only the app that's being deployed should have to worry about configuring this.

In the case of Remix, as a dependency, we don't dynamically require any of our own modules, otherwise they wouldn't be dynamic! The requires are dynamic because we don't know the names of the files: they're supplied by the app. Additionally, apps get to configure where their "data loaders" live, so we wouldn't even be able to specify this information ourselves anyway. They're app modules.

Another case is Prism.js. It does dynamic requires for plugins and themes. If I'm deploying a markdown blog with prism, I would definitely not want prism to be defining every possible file that could be required and then netlify deploying a bunch of code I don't use (and if those modules have side-effects, they'd likely break something).

I think the only thing you need is an include option with an array of glob patterns:

[build]
  functions = "src"
  include = [
    "src/app/remix.config.js",
    "src/app/build/**/*",
    "src/app/node_modules/prisms/plugins/whatever.js"
  ]

Seems a more natural place to configure would be for each function (other platforms I've used have this). I couldn't find anything similar in the docs, so I guess package.json would work?

// functions/some-func/package.json
{
  "netlify": {
    "include": ["node_modules/prismjs/plugins/whatever.js"]
  }
}

[ryanflorence]

Oh, just noticed this recommended directory structure at https://docs.netlify.com/functions/build-with-javascript/#unbundled-javascript-function-deploys

├─ my-base-directory
│  ├─ package.json
│  └─ node_modules
└─ my-serverless-functions
   └─ hello.js
   └─ send-pdf-background.js

If that's the recommend structure, then I think the netlify.toml makes sense. Then the "build bot" mentioned can do its normal thing, and then look at the "include" key in netlify.toml, glob that extra stuff in and it's done!

I don't think it needs to be much more complicated than that 😁

And to reiterate, I really don't think you want third party dependencies able to define behavior of a Netlify customer's deployment config 😬 Too many dragons there with module side-effects and unnecessarily inflating the size of the zip (and therefore cold starts, etc.). Then you'll need an "exclude" option to prevent that stuff, and now it's a deployment battle between the app and the dependencies.

[ryanflorence]

After poking around the code a bit, I think #225 is probably the right approach here :)

[eduardoboucas]

I believe all the problems described here were addressed:

• It's now possible to include any files in the bundle using the included_files configuration property (https://www.netlify.com/blog/2021/08/12/how-to-include-files-in-netlify-serverless-functions/)
• We have better support for imports using dynamic expressions when using esbuild (https://www.netlify.com/blog/2021/04/02/modern-faster-netlify-functions/)

If there's any part of this thread that hasn't been addressed yet, please add a comment and I'll reopen the issue.

Thanks!