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

D1 Binding - Null Support - Fixed by latest worker-build I guess - Added support for handling None on First #680

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

spigaz
Copy link
Contributor

@spigaz spigaz commented Dec 10, 2024

Update: I believe the issue is fixed now!

While I was working on isolating the issue, when I updated the build command of the sandbox to also use cargo install -q worker-build, it took some time to return, and then I was no longer able to reproduce it.

AFAIK its fixed and runs fine!

So the relevance of this PR now is just

  • Test cases for the usage of optional - nullable values, to avoid its return
  • Fix and testcase for first that wasn't able to handle the case when none was returned.

Previously:

As I mentioned on issue #678 the main problem had to do with the JsValue::Null that for some reason, was somehow overwritten by the Env variable.

So on serialisation it serialised the env variable content, and it seems that on serialisation it still used the same env variable to check if its a null or not.

In practice this meant:

  • When it tried to serialise None, it sent the full env as an object
  • When it tried to deserialise, it didn't recognise it as null because it was different from JsValue::NULL, and complained in the context of expecting a Some value.

When I tried to reconstruct manually it deserialised just fine, because here the JsValue::NULL is the one referencing the Env and not JsValue(null).

let js_value = Object::new();
Reflect::set(&js_value, &JsValue::from_str("id"), &JsValue::from_f64(1.0)).unwrap();
Reflect::set(&js_value, &JsValue::from_str("name"), &JsValue::NULL).unwrap();
Reflect::set(&js_value, &JsValue::from_str("age"), &JsValue::NULL).unwrap();

let js_value: JsValue = js_value.into();
let value: NullablePerson = serde_wasm_bindgen::from_value(js_value).unwrap();

@spigaz spigaz changed the title D1 Binding - Null Support - Added tests to isolate the issue and fixed first to return None D1 Binding - Null Support - Fixed by latest worker-build I guess - Added support for handling None on First Dec 11, 2024
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.

1 participant