zip-it-and-ship-it: This does not work with dynamic or conditional module imports

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:

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.
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):

User should npm install the dependency
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.

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 2
Comments: 21 (7 by maintainers)

Commits related to this issue

➕ Add missing optional dep Needed because https://github.com/netlify/zip-it-and-ship-it/issues/68 — committed to mraerino/netbox-deployer by mraerino 5 years ago
Install sodium-native https://github.com/netlify/zip-it-and-ship-it/issues/68 — committed to davedoesdev/mce by davedoesdev 4 years ago
Manually add "encoding" module Due to https://github.com/netlify/zip-it-and-ship-it/issues/68, and because faunadb uses node-fetch, we have to explicitly add encoding as a dependency or the function ... — committed to iansjk/legacy-arknights-tools by iansjk 3 years ago

Most upvoted comments

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

DavidWells on Nov 6, 2019

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.

ryanflorence on Oct 31, 2020

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!

eduardoboucas on Aug 20, 2021

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 on Dec 10, 2020

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

kentcdodds on Nov 9, 2020

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

kentcdodds on Dec 10, 2020

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

kentcdodds on Oct 29, 2020

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!

kentcdodds on Oct 27, 2020