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

feat: explicit loading of native binaries #127

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

belgattitude
Copy link
Contributor

@belgattitude belgattitude commented Jan 29, 2025

Proposal to fix: #126

Before this change, the loading of node js binary was fully dynamic.

The proposed way to do this is to explicitly define all binaries that could be loaded. That helps framework that applies tracing on node_modules such as nextjs (when deployed as lambda or standalone) to detect that the binary is in use. It also allows to set a custom message indicating the reason of error.

  • Is the proposed way good enough ?
  • What error message to display when an architecture isn't supported ?

There's many other ways to achieve the same idea (see esbuild, or sharp with require.resolve), this one keeps the loading as simple possible.

Happy to discuss if it makes sense

Copy link
Contributor

@jraymakers jraymakers left a comment

Choose a reason for hiding this comment

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

A few minor questions/comments about the code patterns. The overall approach makes sense.

@belgattitude
Copy link
Contributor Author

belgattitude commented Jan 30, 2025

@jraymakers I've updated the PR. Didn't went to the nested switch though :)

Hope it can get merged soon, I'm really excited by duckdb neo so far.

@jraymakers
Copy link
Contributor

This can probably be simplified further (I suspect the function wrapping the string interpolation, and the later string split, are not necessary), but it should work fine as-is. Thanks for the contribution!

@jraymakers jraymakers merged commit 1cac29e into duckdb:main Jan 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[node-bindings] Nextjs standalone output support
2 participants