-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
webassembly: Revert conversion of Python None to JavaScript null, and make dict proxy return undefined when key doesn't exist #14483
webassembly: Revert conversion of Python None to JavaScript null, and make dict proxy return undefined when key doesn't exist #14483
Conversation
@WebReflection please take a look at this PR and see what you think. It's a bit different to what we discussed on Discord, the difference being that |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14483 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 161 161
Lines 21204 21204
=======================================
Hits 20864 20864
Misses 340 340 ☔ View full report in Codecov by Sentry. |
ports/webassembly/proxy_c.c
Outdated
@@ -228,7 +241,15 @@ void proxy_c_to_js_lookup_attr(uint32_t c_ref, const char *attr_in, uint32_t *ou | |||
qstr attr = qstr_from_str(attr_in); | |||
mp_obj_t member; | |||
if (mp_obj_is_dict_or_ordereddict(obj)) { | |||
member = mp_obj_dict_get(obj, MP_OBJ_NEW_QSTR(attr)); | |||
// Lookup the requested attribute as a dict key (not a method), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that not as method
kinda bothers me ... function some({ thing = () => {} } = {}) {}
is a valid JS expectation, if this is covered in here I am OK with that comment (if explained somewhere else), otherwise I wonder "why"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just that the comment I wrote here is confusing. I meant that here the code is looking up the attribute as a key within the dict (or ordereddict). That's opposed to the standard case of looking up the attribute as the name of a member/function/method of the target object.
Eg looking up attr
on target
will have the following logic:
- If
target
is adict
orordereddict
instance then lookuptarget[attr]
, and return the result. If the lookup failed, returnjs.undefined
. - Otherwise, lookup
target.foo
, raising an exception if it can't be found.
I've updated the comments to try and make it clearer.
I also added a test for passing through a function in the dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, 👍 for me!
it's true that methods (or utilities?) are rare, but not that much neither for JS related APIs ... I've left a comment that, once resolved or explained, would make me virtually approve this PR as I think it's awesome in both intent and expectations 👍 |
ede7f72
to
22cb120
Compare
This reverts part of commit fa23e4b, to make it so that Python `None` converts to JavaScript `null` (and JavaScript `null` already converts to Python `None`). That's consistent with how the `json` module converts these values back and forth. Signed-off-by: Damien George <damien@micropython.org>
This adds a new undefined singleton to Python, that corresponds directly to JavaScript `undefined`. It's accessible via `js.undefined`. Signed-off-by: Damien George <damien@micropython.org>
Instead of raising KeyError. These semantics match JavaScript behaviour and make it much more seamless to pass Python dicts through to JavaScript as though they were JavaScript {} objects. Signed-off-by: Damien George <damien@micropython.org>
22cb120
to
cfd5a8e
Compare
There are 3 related commits in this PR:
None
convert toundefined
. So now it's back to how it was before that commit, and PyNone
converts to JSnull
. That's consistent with how thejson
module converts these values.undefined
. It's accessible viajs.undefined
. Passing this from Py to JS, JS will receiveundefined
. And when JS passesundefined
to Py, Py will see the proxyjs.undefined
value.undefined
to JS when JS looks up a key that doesn't exist (instead of raisingKeyError
). These semantics match better the JS behaviour of{}
objects.The first two changes mean the following:
None
and JSnull
.js.undefined
and JSundefined
.json
module.All three changes together mean that, given the following JavaScript function with an optional argument:
the JavaScript and Python code to call this function have equivalent behaviour, namely, in JavaScript:
and in Python: