-
Notifications
You must be signed in to change notification settings - Fork 131
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
base: main
Are you sure you want to change the base?
Refactor react-duckdb #1727
Conversation
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 |
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? |
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
export * from './connection_provider'; | |
export * from './database_provider'; | |
export * from './platform_provider'; | |
export * from './resolvable'; |
|
||
const DuckDBContext = createContext<DuckDBContextValue | null>(null); | ||
|
||
// TODO: consider adding support for passing in an existing AsyncDuckDB instance |
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
The existing
react-duckdb
providers 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.