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: loading of duckdb every query #4903

Merged
merged 4 commits into from
Jan 3, 2023
Merged

fix: loading of duckdb every query #4903

merged 4 commits into from
Jan 3, 2023

Conversation

ozkatz
Copy link
Collaborator

@ozkatz ozkatz commented Dec 29, 2022

Fix the duckDB wasm package being downloaded on every query (as well as a few minor tweaks to the UX of running said queries)

@ozkatz ozkatz added bug Something isn't working include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached labels Dec 29, 2022
@ozkatz ozkatz requested a review from eladlachmi December 29, 2022 12:49
@ozkatz ozkatz assigned nopcoder and unassigned nopcoder Dec 29, 2022
@ozkatz ozkatz requested a review from nopcoder December 29, 2022 14:59
Copy link
Contributor

@eladlachmi eladlachmi 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!
Just a few relatively minor comments.

const data = results.toArray()
setData(data)
setError(null)

}).catch(e => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're mixing paradigms. .catch is Promise syntax and async-await is a different approach. If you're using async-await, you should use plain old try-catch-finally. That's the idea of async-await - writing sync-style code, and abstracting away all of the Promise handling, continuation, error handling, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

let content;
let button = (
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO something along these lines would be cleaner and more readable.

Suggested change
let button = (
const button = (
<Button type="submit" variant="success" disabled={loading}>
<DatabaseIcon /> {" "}
{ loading ? "Executing..." : "Execute" }
</Button>
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice! used.

e.preventDefault()
setShouldSubmit(!shouldSubmit)
}}>
<Form onSubmit={handleSubmit}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Like! 🤩

<Form.Control as="textarea" className="object-viewer-sql-input" rows={5} value={query} spellCheck={false} onChange={e => {
setQuery(e.target.value)
}} />
<Form.Control as="textarea" className="object-viewer-sql-input" rows={5} defaultValue={initialQuery} spellCheck={false} ref={sql} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's something you want, but maybe add autocomplete={false}, so the native browser autocomplete thing doesn't pop up (unless you want. that, of course)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added!

</Form>
<div className={"mt-3 object-viewer-sql-results"}>
<div className={"mt-3"}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure there's a need for the curly braces

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

@ozkatz ozkatz requested a review from eladlachmi January 3, 2023 13:40
Copy link
Contributor

@eladlachmi eladlachmi left a comment

Choose a reason for hiding this comment

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

LGTM

@ozkatz ozkatz merged commit c482741 into master Jan 3, 2023
@ozkatz ozkatz deleted the fix/duckdb-wasm-loading branch January 3, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants