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

[miniflare D1] replaces returnsData in statement with a natively supported function #533

Merged

Conversation

jschlesser
Copy link
Contributor

I would like to point out that while this PR simplifies the codebase and makes it less fragile and implements the actual intent and is probably better for the longer term, it may not be the exact same behavior. However, its not dependent on an error message changing and it will avoid issues arising from TypeError not working working with instanceof comparisons in certain uses of the vm api to execute code. Also D1 is technically beta 🤷

@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2023

⚠️ No Changeset found

Latest commit: beb935b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jschlesser
Copy link
Contributor Author

References this issue

@jschlesser
Copy link
Contributor Author

Also note that this property is a direct pass through to the native sqlite returns_data method
source code reference

The original D1 miniflare returnsData method could technically throw an exception, It's not clear to me that this patch ever would.

@jschlesser jschlesser changed the title replaces returnsData with a natively supported function [miniflare D1] replaces returnsData in statement with a natively supported function Mar 5, 2023
@mrbbot
Copy link
Contributor

mrbbot commented Mar 5, 2023

Hey! 👋 Thanks so much for the PR! This is exactly what we were looking for! 😃

it may not be the exact same behavior.

I'm not too concerned about that here, as you point out D1 isn't stable yet so we have flexibility.

@mrbbot mrbbot merged commit cee791d into cloudflare:master Mar 6, 2023
mrbbot added a commit that referenced this pull request Apr 26, 2023
Also brings forward the changes from #533 and #544

Closes DEVX-591
mrbbot added a commit that referenced this pull request Apr 28, 2023
Also brings forward the changes from #533 and #544

Closes DEVX-591
mrbbot added a commit that referenced this pull request May 9, 2023
* Re-implement D1 gateway using new storage system

Also brings forward the changes from #533 and #544

Closes DEVX-591

* fixup! Re-implement D1 gateway using new storage system
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.

2 participants