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 sql.js out of SQLJSPackageDB constructor #45

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Conversation

cmoesel
Copy link
Member

@cmoesel cmoesel commented Dec 12, 2024

Description: Instead of requiring the caller to initialize and pass in a sql.js Database, add an async initialize() function to do it within the SQLJSPackageDB implementation.

Callers can also use the new async newSQLJSPackageDB funtion to get a new initialized SQLJSPackageDB (one call instead of two).

Testing Instructions: Ensure tests run. Try CLI.

Related Issue: Done in response to this comment on the SUSHI PR integrating FPL 2.0.

Instead of requiring the caller to initialize and pass in a sql.js Database, add an async initialize() function to do it within the SQLJSPackageDB implementation.

Callers can also use the new async newSQLJSPackageDB funtion to get a new initialized SQLJSPackageDB (one call instead of two).
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

I like this new approach! I think it makes things simpler for anyone (SUSHI) who is using FPL. I left a couple comments and a couple questions just for my own understanding.

src/db/SQLJSPackageDB.ts Outdated Show resolved Hide resolved
src/db/SQLJSPackageDB.ts Show resolved Hide resolved
test/db/SQLJSPackageDB.test.ts Outdated Show resolved Hide resolved
test/db/SQLJSPackageDB.test.ts Show resolved Hide resolved
Copy link
Collaborator

@jafeltra jafeltra 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! Thanks for answering my questions!

@cmoesel cmoesel merged commit 014d710 into main Dec 13, 2024
14 checks passed
@cmoesel cmoesel deleted the sushi-pr-updates branch December 13, 2024 16:11
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.

3 participants