-
Notifications
You must be signed in to change notification settings - Fork 168
Refactor react-duckdb #1727
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
Refactor react-duckdb #1727
Conversation
ncsmikewoods
commented
Jun 21, 2024
I've been keeping an eye on these changes. It looks like they'll address some problems with race conditions in duckdb initialization. I've been holding off rewriting my connection management code because this refactor looks like a much cleaner implementation that's what's there.
Thanks for your work @willium
jr200
commented
Sep 20, 2024
This patch would be helpful to me too. Are there plans to merge and release this PR? Thanks!
I have to say I don't feel very competent to review this PR, would someone volunteer to have a deeper look?
@qw-in
qw-in
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like index.ts is still referencing the deleted files here
duckdb-wasm/packages/react-duckdb/src/index.ts
Lines 1 to 4 in cda56b4
@qw-in
qw-in
Oct 1, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 0ドル.02 is it seems like most of the complexity is to handle the bundle, bundles, etc
I think this is a wise comment, and I even wonder if supporting only an existing AsyncDuckDB instance makes sense
Uh oh!
There was an error while loading. Please reload this page.
The existing
react-duckdbproviders were pretty convoluted and had properties that it didn't use. I refactored it and added documentation and comments.I did not bump the version in
package.json. This is a breaking change for this package, so I suspect it should be (part of?) a major version bump.I may contribute some utility functions/additional hooks in the future, but these shouldn't cause breaking changes.