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(cli): Add esbuild loader for .node files for codebase #4210

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

LewisJEllis
Copy link
Contributor

@LewisJEllis LewisJEllis commented Aug 8, 2024

To hopefully make scripts depending on e.g. ssh2 work

Related issues suggesting this fix:

Without this, bundling a script that depends on ssh2 results in something like:

Error: R] Could not resolve "../build/Release/cpufeatures.node"

    ../node_modules/.pnpm/cpu-features@0.0.10/node_modules/cpu-features/lib/index.js:3:24:
      3 │ const binding = require('../build/Release/cpufeatures.node');
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

or:

✘ [ERROR] No loader is configured for ".node" files: lib/testfunc/node_modules/cpu-features/build/Release/cpufeatures.node

    lib/testfunc/node_modules/cpu-features/lib/index.js:1:24:
      1 │ const binding = require('../build/Release/cpufeatures.node');
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

✘ [ERROR] No loader is configured for ".node" files: lib/testfunc/node_modules/ssh2/lib/protocol/crypto/build/Release/sshcrypto.node

    lib/testfunc/node_modules/ssh2/lib/protocol/crypto.js:30:20:
      30 │   binding = require('./crypto/build/Release/sshcrypto.node');
         ╵                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

🚀 This description was created by Ellipsis for commit d3973d5

Summary:

Added .node file loader to esbuild configuration in cli/script.ts to fix bundling issues with native modules like ssh2.

Key points:

  • Added .node file loader to esbuild configuration in cli/script.ts.
  • Modified handleFile function to include { ".node": "file" } in loader option.
  • Fixes bundling issues for scripts using native modules like ssh2.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

github-actions bot commented Aug 8, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to d3973d5 in 27 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. cli/script.ts:170
  • Draft comment:
    The addition of the .node file loader configuration is appropriate for handling native modules in the bundling process. This change should resolve the issues with dependencies like ssh2 that rely on native node modules.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR adds a loader configuration for .node files to the esbuild build options in the handleFile function. This is intended to resolve issues with bundling scripts that depend on native node modules, as described in the PR description. The loader configuration is set to treat .node files as file, which means esbuild will copy these files to the output directory instead of trying to bundle them, which is appropriate for native modules that cannot be bundled like regular JavaScript files.

Workflow ID: wflow_fMmN70Wa4tNhuRDE


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@LewisJEllis
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Aug 8, 2024
@rubenfiszel rubenfiszel changed the title Add esbuild loader for .node files fix: Add esbuild loader for .node files Aug 8, 2024
@rubenfiszel rubenfiszel changed the title fix: Add esbuild loader for .node files fix(cli): Add esbuild loader for .node files Aug 8, 2024
@rubenfiszel rubenfiszel changed the title fix(cli): Add esbuild loader for .node files fix(cli): Add esbuild loader for .node files for codebase Aug 8, 2024
@rubenfiszel rubenfiszel merged commit a77f74c into windmill-labs:main Aug 8, 2024
1 check passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants