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

zipFunction does not bundle @aws-sdk/client-s3 library #5708

Open
dsope05 opened this issue Jun 12, 2024 · 7 comments
Open

zipFunction does not bundle @aws-sdk/client-s3 library #5708

dsope05 opened this issue Jun 12, 2024 · 7 comments
Labels
type: bug code to address defects in shipped code

Comments

@dsope05
Copy link

dsope05 commented Jun 12, 2024

Describe the bug

zipFunction w/ esbuild (nodeBundler) does not bundle npm library @aws-sdk/client-s3.

instead, it just includes the import statement in the output bundle file:

also adding @aws-sdk/client-s3 to the externalNodeModules option does not include the node_module in the output folder.

Steps to reproduce

  1. create new folder and npm init, change to "type": "module" in package.json.

  2. install @aws-sdk/client-s3, and @netlify/zip-it-and-ship-it

  3. create simple index.js file that imports and logs @aws-sdk/client-s3
    import sdk from '@aws-sdk/client-s3'; const a = () => { console.log('sdk', sdk) } export default a;

  4. create zipit.js file that calls zipFunction with zipConfig

import { zipFunction } from '@netlify/zip-it-and-ship-it'; const zipConfig = { nodeBundler: 'esbuild', externalNodeModules: ['@aws-sdk/client-s3'] }; zipFunction('index.js', 'zip-out', { archiveFormat: 'none', config: { '*': zipConfig, }, }); };
5. open output file zip-out/index/index.mjs
`
import {createRequire as ___nfyCreateRequire} from "module";
import {fileURLToPath as ___nfyFileURLToPath} from "url";
import {dirname as ___nfyPathDirname} from "path";
let __filename=___nfyFileURLToPath(import.meta.url);
let __dirname=___nfyPathDirname(___nfyFileURLToPath(import.meta.url));
let require=___nfyCreateRequire(import.meta.url);

// index.js
import sdk from "@aws-sdk/client-s3";
var a = () => {
console.log("sdk", sdk);
};
var testy_default = a;
export {
testy_default as default
};
`

Result: The @aws-sdk/client-s3 is not bundled, nor is it included in any parallel node_modules in the zip-out folder.

CLI command and flags

node zipit.js

Configuration

No response

CLI output

this is the output of the zipFunction // zip-out/index/index.mjs

`
import {createRequire as ___nfyCreateRequire} from "module";
import {fileURLToPath as ___nfyFileURLToPath} from "url";
import {dirname as ___nfyPathDirname} from "path";
let __filename=___nfyFileURLToPath(import.meta.url);
let __dirname=___nfyPathDirname(___nfyFileURLToPath(import.meta.url));
let require=___nfyCreateRequire(import.meta.url);

// index.js
import sdk from "@aws-sdk/client-s3";
var a = () => {
console.log("sdk", sdk);
};
var testy_default = a;
export {
testy_default as default
};
`

Environment

System:
OS: macOS 14.2.1
CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Memory: 773.20 MB / 32.00 GB
Shell: 5.9 - /bin/zsh
Binaries:
Node: 18.17.0 - ~/.nvm/versions/node/v18.17.0/bin/node
npm: 9.6.7 - ~/.nvm/versions/node/v18.17.0/bin/npm
pnpm: 8.6.12 - ~/Library/pnpm/pnpm
bun: 1.0.6 - ~/.bun/bin/bun

@dsope05 dsope05 added the type: bug code to address defects in shipped code label Jun 12, 2024
@saikiran1043
Copy link

saikiran1043 commented Jun 12, 2024

We see Readme.md says @aws-sdk/* is excluded on node18 and later only for zisi bundler, but it is getting excluded for esbuild as well. Is the readme incorrect (or) its the code issuePlease let us know if there is any specific reason to exclude that package in node18 and later for esbuild?
Readme link: https://github.com/netlify/build/tree/e31f6da5d8d9f4dbaf70798a04d429dba72928d9/packages/zip-it-and-ship-it#zisi

esbuild/special_cases link: https://github.com/netlify/build/blob/main/packages/zip-it-and-ship-it/src/runtimes/node/bundlers/esbuild/special_cases.ts#L23

@Skn0tt
Copy link
Contributor

Skn0tt commented Jun 13, 2024

zip-it-and-ship-it is built to deploy functions to AWS Lambda. Lambda includes the AWS Sdk into the Node.js runtime already, so we exclude it from the bundle. This ensure that your Functions are always using the SDK version that AWS gives you, and it reduces the bundle size. See https://docs.aws.amazon.com/lambda/latest/dg/lambda-nodejs.html for docs on this.

@saikiran1043
Copy link

Hi @Skn0tt, thanks for the information. We are trying to use zip-it-and-ship-it in non AWS Lambda environment. Do you think we can have some kind of configuration to request to include @aws-sdk/* packages as well in bundle?

@saikiran1043
Copy link

Hi @Skn0tt, thanks for the information. We are trying to use zip-it-and-ship-it in non AWS Lambda environment. Do you think we can have some kind of configuration to request to include @aws-sdk/* packages as well in bundle?

Hi @Skn0tt , can you please share your thoughts on the above

@Skn0tt
Copy link
Contributor

Skn0tt commented Jun 21, 2024

That’s out of scope for this project right now, but i’ll let @eduardoboucas speak to this.

@saikiran1043
Copy link

Thank you @Skn0tt for your prompt response and for clarifying the project's scope. I understand that the feature we requested is currently out of scope.

Would you be open to including this feature if we contribute to the implementation? The approach I am planning to take is to add an additional parameter (e.g., includeAWSSDK: true/false) to the options/config object, allowing esbuild users to include the package without any other impact.

zipFunctions(srcFolders, destFolder, options?)
eg:-
zipFunctions(srcFolders, destFolder, {
    config: {
      'includeAWSSDK': true,
    },
  });

Please let me know if this approach is acceptable and if you would be open to including this feature within the project's scope. I am happy to contribute to the implementation and submit a pull request.

@saikiran1043
Copy link

Thank you @Skn0tt for your prompt response and for clarifying the project's scope. I understand that the feature we requested is currently out of scope.

Would you be open to including this feature if we contribute to the implementation? The approach I am planning to take is to add an additional parameter (e.g., includeAWSSDK: true/false) to the options/config object, allowing esbuild users to include the package without any other impact.

zipFunctions(srcFolders, destFolder, options?)
eg:-
zipFunctions(srcFolders, destFolder, {
    config: {
      'includeAWSSDK': true,
    },
  });

Please let me know if this approach is acceptable and if you would be open to including this feature within the project's scope. I am happy to contribute to the implementation and submit a pull request.

Hi @Skn0tt / @eduardoboucas , can you please share your thoughts on the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

No branches or pull requests

3 participants