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 #270

Closed
wants to merge 18 commits into from
Closed

Conversation

FlareLine
Copy link
Contributor

@FlareLine FlareLine commented Jan 20, 2023

Summary

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

Due to the nature of local development with Rust workers, developing an easy process for locally debugging this integration may be somewhat difficult.

Attributions:

  • @KernelFreeze for initial contributions to the feature.
  • @SebastiaanYN for assistance with the query! macro.
  • @logankeenan for working on the integration tests.
  • @zebp for all the work they do on the crate, despite their massive existing workload.

@FlareLine
Copy link
Contributor Author

After some messing about in both wrangler2 and workers-rs, I've finally gotten multiple parameters binding to a prepared statement. This should be the last change required for this implementation bar tests / bit of cleanup.

@FlareLine FlareLine mentioned this pull request Jan 26, 2023
3 tasks
@FlareLine FlareLine marked this pull request as ready for review January 26, 2023 23:47
@FlareLine
Copy link
Contributor Author

FlareLine commented Jan 27, 2023

There's actually still a couple of things I need to add - I'll get these in hopefully in the next couple of days.

Copy link
Collaborator

@zebp zebp left a comment

Choose a reason for hiding this comment

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

Looks really good, but we need to get some testing in worker-sandbox.

worker/src/env.rs Outdated Show resolved Hide resolved
worker/src/lib.rs Outdated Show resolved Hide resolved
@FlareLine FlareLine dismissed a stale review via 51a329e February 2, 2023 06:26
@FlareLine FlareLine dismissed a stale review via c725fcb February 2, 2023 06:58
worker/src/d1.rs Outdated Show resolved Hide resolved
@FlareLine FlareLine dismissed a stale review via 419039b February 10, 2023 13:41
@FlareLine FlareLine requested review from SebastiaanYN and zebp and removed request for SebastiaanYN and zebp February 10, 2023 13:51
@FlareLine
Copy link
Contributor Author

Apologies for the above, Github is funky...

@FlareLine FlareLine dismissed a stale review via 68e9cd0 February 14, 2023 10:41
@FlareLine
Copy link
Contributor Author

Apologies for the delay, it's been a crazy couple of weeks. I'm currently writing some tests for this integration using the worker-sandbox crate. Hopefully this PR will be ready for review in the next couple of days. 🙏 cc @zebp

@FlareLine
Copy link
Contributor Author

After attempting to write some tests for this integration. I have the pattern down pat. However, it seems like Miniflare doesn't like the types specified in worker... I'm not entirely sure how to solve this:
image

@FlareLine
Copy link
Contributor Author

FlareLine commented Mar 19, 2023

@FlareLine

I used this branch...

Hey @logankeenan,

Hmm interesting - I had hopefully gotten rid of any problems related to errors at this point. I will spend some time coming up with an interface that allows us to extract the cause of the error. Unfortunately (at the moment) Wrangler wraps the error cause up into the cause property of the thrown javascript Error rather than in the error message itself, and clearly my changes in error.rs aren't working to unwrap this as intended. I'll have a play around and see if I can work it out.

@FlareLine FlareLine dismissed a stale review via 3081e2c March 19, 2023 22:10
@FlareLine
Copy link
Contributor Author

@logankeenan interestingly, I had to fix some issues with the macro after @zebp's changes to naming of the D1 interface. Macros are powerful, but can be quite fiddly when it comes to refactoring! I've pushed it up to CF to test and it seems to work okay, but I'm yet to test it locally. Would you mind giving this a go with the fixes and report back? :)

@logankeenan
Copy link

@FlareLine - My repo is now working as expected. However, I'm embarrassed to say it's because I'm now passing the --persist flag when running npx wrangler dev --local --persist 🤦. Your recent changes were still not working, so I re-read the documentation and realized my mistake. I'm sorry for the false positive!

@logankeenan
Copy link

@FlareLine

I've tested this branch quite a bit through a demo application. Is there anything I can help with on this PR?

@FlareLine
Copy link
Contributor Author

FlareLine commented Apr 4, 2023

@FlareLine

I've tested this branch quite a bit through a demo application. Is there anything I can help with on this PR?

Hey @logankeenan, I believe there were some conversations happening between @zebp and the D1 team regarding some questions. I'm not sure where those conversations are at. 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.

Apologies for the late reply.

@logankeenan
Copy link

@FlareLine

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 can give the PR a look and see where more tests could be added. Take it easy and I hope you get better soon!

@FlareLine
Copy link
Contributor Author

@FlareLine

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 can give the PR a look and see where more tests could be added. Take it easy and I hope you get better soon!

@FlareLine FlareLine closed this Apr 6, 2023
@Smephite
Copy link

Smephite commented Apr 6, 2023

Why was this PR closed?

@FlareLine FlareLine reopened this Apr 7, 2023
@FlareLine
Copy link
Contributor Author

FlareLine commented Apr 7, 2023

Why was this PR closed?

Uhh no idea, must have fumbled the bag and clicked close with comment - reopened, my apologies! 😅

@FlareLine FlareLine dismissed a stale review via c35d762 April 7, 2023 00:26
@alpancs
Copy link

alpancs commented May 4, 2023

hi guys, thanks for creating this PR, but any updates on it? I think all the issues have been resolved.

@logankeenan
Copy link

I forked this branch and created a new PR since the original author is unavailable.

@FlareLine
Copy link
Contributor Author

I forked this branch and created a new PR since the original author is unavailable.

Hey there! I've been attempting to see if I can get a hold of @zebp as there were some conversations he was having with the D1 team. I'm not entirely sure where those conversations are at. I think at the moment it's been fairly busy at Cloudflare, which is probably why it's been difficult to get time to take a look at getting this across the line.

@logankeenan
Copy link

@FlareLine

👋 Good to hear from you. Thanks for the update. Let me know how you'd like me to proceed. I'd be happy to close the PR I just opened. Whatever is easiest.

@FlareLine FlareLine dismissed a stale review via f018e12 May 5, 2023 01:29
@FlareLine
Copy link
Contributor Author

@logankeenan I've just cherry-picked your integration test commit into this PR! Appreciate the work!

@zebp
Copy link
Collaborator

zebp commented Jun 14, 2023

Closing in favor of #337

@zebp zebp closed this Jun 14, 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.

6 participants