feat(runtime-sdk): revise type conversion to support bindings more natively#112
feat(runtime-sdk): revise type conversion to support bindings more natively#112ryanking13 wants to merge 7 commits into
Conversation
…flare bindings more natively - Python dict is now converted to js object by default when passed through the RPC boundary. - JavaScript Null object is now converted to Python None when coming from RPC boundary. - JavaScript Object coming from RPC boundary is now converted to Python-dict like object but their attributes are accessible through ["key"] but also by .key - Better support for R2, KV, and D1 bindings support
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds native SDK support for KV, R2, and D1 bindings by revising type conversion (dict→Object, jsnull→None) and introducing _RPCWrapper / JsDict for ergonomic access patterns.
Issues (by severity)
-
_RPCWrapper.__getattr__caches wrappers for non-callable properties, causingself.__class__to propagate the wrong subclass. When_D1DatabaseWrapper._convert_resultreceives a JsProxy (e.g., aD1PreparedStatementfrom.prepare()), it wraps it as_D1DatabaseWrapper(result)becauseself.__class__is_D1DatabaseWrapper. The prepared statement then carries the custombindoverride. This happens to work for D1, but if any other non-D1 JsProxy is returned from a D1Database method in the future, it would incorrectly get thebindoverride. Consider using_RPCWrapperexplicitly for results that aren'tD1Databaseinstances, or check the JS constructor name before choosing the wrapper class. -
JsDicthas asymmetric read/write behavior for attribute access.d.fooreads from the dict (via__getattr__), butd.foo = valwrites to the instance__dict__(not the dict) since there's no__setattr__override. This meansd.foo = 'bar'followed byd['foo']will return the old value. SinceJsDictis used for read-only result objects this is likely fine in practice, but it could surprise users who try to mutate via attribute syntax. Consider adding a__setattr__that also writes to the dict, or documenting this as read-only. -
Missing newline at end of file in
d1_test.pyandr2_test.py.
|
I'm Bonk, and I've done a quick review of your PR. This PR adds native SDK support for KV, R2, and D1 bindings by revising type conversion (dict→Object, jsnull→None) and introducing I posted a review on PR #112 with 4 inline comments covering 3 issues ranked by severity:
The test coverage is thorough — good coverage of KV (20 tests), R2 (20 tests), and D1 (18 tests) including edge cases like pagination, multipart uploads, and null binding. |
This is great, though the ideal would be to make the following work: I think this would be the most Pythonic option. Though I understand why it might be challenging. I think it's worth discussing.
This is great, and I think it makes perfect sense. Is this something we can upstream to Pyodide as well?
What is surprising to me is actually that Pyodide converts Is there a path for us to change this? Perhaps with some feature flag in Pyodide so that existing Pyodide users aren't broken by this? |
Acutally, this already works. Pyodide converts keyword parameters to JS object as a last parameter, which means the following two are equivalent. I added some new tests to make sure if works (See: e202e8d) I am not sure which one we should document though. Maybe we can discuss this to make sure we have all the examples in our documentation to have the same format
Yeah, we already have something similar:
However, the problem is that the existing Python workers that run Pyodide 0.28.2 would still have jsnull so we need to keep this conversion logic unless we drop supporting old Python workers in our sdk.
Well, I think it makes sense to convert |
Why? |
Note: this includes some breaking changes so it needs a careful look. Also this probaby needs to be merged after #108.
I was experimenting with our current type conversion logic in our SDK to see if a small change is sufficient to make the bindings work more natively. With these changes, I was able to make all the general use cases of storage bindings (R2, KV, D1) work without exposing any Pyodide/JS-specific APIs.
Changes included
This allows python dict to be passed to the RPC function without using
to_js. For example,This change was actually landed in Pyodide 0.29.0, but was not applied in Python workers.
Nullis now converted to PythonNonewhen coming from RPC boundary.So that users do not need to care about jsnull, None difference.
["key"]but also by.key(optional)(This change is not necessary, but I think it improves the user experience)
This allows mixing
.key-style access and["key"]-style access. This helps accessing attributes when the RPC returns complicated, nested object with JsProxy and python dict mixed.For example, let's say there is a function that returns the following JS object.
When it passes through the RPC bounday, it will be converted to a Python dict that is
{ "key": str, "data": JsProxy, }where Blob is a JsProxy or a Python class that wrap the JsProxy.
In this case, we can now access the
Blobattribute withwhile previously, users needed to
D1.bindfunction acceptsnullas a parameter. Passingundefinedinstead ofnullis a runtime error. To handle this conversion automatically, I made a wrapper function for.bindto handle this conversion in the background. This means when there are other bindings that needs to handle thenullvalue, we need to create a wrapper function likewise.Alternatively, we can export
jsnulltype to the userse.g. from workers import jsnulland let them pass it to the function, but it makes the user experience less ergonomic.Changes not included
All the JS function names / parameter names are used as-is, as a camelCase. We do not convert it to Pythonic names.