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

feat: Added D1 Support #314

Closed
wants to merge 17 commits into from

Conversation

logankeenan
Copy link

This is a continuation of #270.

@FlareLine made the following statement the existing PR:

I'd be more than happy if you wanted to contribute some more tests to the PR - I've unfortunately come down with something so haven't had time to come back to this.

I made a PR on his fork, but it haven't had a response. I'll see if I can moe this PR along and would be happy to close this PR if they can come back.

Summary

These changes add D1 support to the worker create, resolving #221.

I've added some integration tests to worker-sandbox and left a few comments to see how to proceed. @zebp let me know your thoughts and thank you for your time.

}

#[test]
fn d1_select_prepare_all() {
Copy link
Author

@logankeenan logankeenan May 5, 2023

Choose a reason for hiding this comment

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

I tried to highlight the API that is being used in the tests, but I think the test/fn names would be better if they were more verbose

}

/// Executes a query against the database and returns a `Vec` of rows instead of objects.
pub async fn raw<T>(&self) -> Result<Vec<Vec<T>>>
Copy link
Author

Choose a reason for hiding this comment

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

I don't think it should take a generic type T because consumers will unlikely write a select statement where all the types are the same. In the people case, select * from my table will cause a type error between the String, Integer or other types defined in the table's column. Perhaps, it should be a String rather than T?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true - I'll take a look. I think this was supposed to be a Result<Vec<T>>, but not sure.

/// )?;
/// ```
#[macro_export]
macro_rules! query {
Copy link
Author

Choose a reason for hiding this comment

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

I haven't written any tests for this, but could

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't test all functionality yet, but I wanted to see what thoughts are on my approach.

@FlareLine
Copy link
Contributor

Hey @logankeenan! I've just commented on the original PR - I would be happy to merge these changes into the main PR if you're happy with that? I believe there's still some conversations going on with the D1 team, which is why the PR hasn't been merged / worked on recently.

@logankeenan
Copy link
Author

Hey @logankeenan! I've just commented on the original PR - I would be happy to merge these changes into the main PR if you're happy with that? I believe there's still some conversations going on with the D1 team, which is why the PR hasn't been merged / worked on recently.

That sounds good to me, thank you!

@logankeenan logankeenan closed this May 5, 2023
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