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

Refactor react-duckdb #1727

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor react-duckdb #1727

wants to merge 4 commits into from

Conversation

willium
Copy link

@willium willium commented May 4, 2024

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.

@carlopi carlopi self-requested a review May 5, 2024 01:49
@ncsmikewoods
Copy link

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
Copy link

jr200 commented Sep 20, 2024

This patch would be helpful to me too. Are there plans to merge and release this PR? Thanks!

@carlopi
Copy link
Collaborator

carlopi commented Sep 20, 2024

I have to say I don't feel very competent to review this PR, would someone volunteer to have a deeper look?

Copy link

@qw-in qw-in left a 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

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
Copy link

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

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.

5 participants