Make `to_js` of a dictionary return a `LiteralMap` #4576

hoodmane · 2024-02-28T02:55:46Z

This acts like both a Map and an Object. Resolves #4030.

Add a CHANGELOG entry
Add / update tests
Add new / update outdated documentation

This acts like both a Map and an Object. Co-authored-by: Andrea Giammarchi <andrea.giammarchi@gmail.com>

WebReflection · 2024-02-28T08:36:36Z

src/js/literal-map.js

+  has: (map, k) => map.has(k) || k in map,
+  ownKeys: (map) =>
+    [...map.keys(), ...ownKeys(map)].filter((x) =>
+      ["string", "symbol"].includes(typeof x),


looks like this is a good thing to do but I wonder why this file is mostly a copy/paste of the library as opposite of being the library. It's harder for me to track possible discrepancies if this gets adopted by other interpreters too.

As this and the constructor check seems to be the only difference, and both make sense to me, would you consider using the library if I reflect those changes in there? 🤔

btw, this might break methods explicitly defined (via defineProperty/ies) and I am not super sure why the filter is needed ... I'd love to understand more, if possible, thank you!

This filter is needed because the ownKeys trap raises if the list returned contains anything that is not a string or symbol.

Makes sense, I’ve mistaken the key with its value, although is not clear to me when a literal key wouldn’t be a string or symbol but I see this playing a role in the dictionary key case.

would you agree this should rather be upstream in the source lib? 🤔

I will implement this ASAP, with or without your consent 😉

FYI this change (only this one + the constructor one as I have concerns about the retro-shadowing) landed and I think it should now work out of the box without surprises.

WebReflection · 2024-03-01T08:06:10Z

src/js/literal-map.js

+  get(map, k, proxy) {
+    if (k === _) return map;
+    let v = map[k];
+    if (typeof v === "function" && k !== "constructor") {


this is shadowing explicit get and set methods on the literal ... there are not many use cases for these or known APIs but properties descriptors have both get and set possible.

I have explained in the README why the module instead prefers own properties over inherited methods, also because that's how it works in general in JS via prototypal inheritance.

a {get: 123} will also fully break here expectations.

Please re-consider this change as I am not sure it should land in my module too.

FYI MicroPython is also considering to switch to this module to make the Python/JS story more portable across interpreters micropython/micropython#13583 (comment) but if this interpreter decides that {"get": py_method} should break, when explicit, in favor of the underlying Map.prototype.get I think my proposal would fail if not used as-is and prefer other interpreters to just convert by default as object literals, differently from Pyodide ... let's please try to find out the best of both worlds we can offer, considering the current de-facto standard in JS APIs is that config options and parameters are expected to be object literals and that a destructured get in a Map would hardly fail no matter what if expected as value or non-context dependent method due new or foreign JS API design, thank you!

ryanking13

Thanks for working on this!

I agree with WebReflection that it's better to use the literal-map library directly if possible, rather than copying an implementation. If we need to make our implementations different for some reason, let's figure out how to make it easy to tell the difference.

WebReflection · 2024-03-07T17:49:25Z

FYI today, Tufts in Boston, we had another weird (to explain) use case: a JS module used as configuration (data and/or options) that gets converted into a Python dictionary and then it cannot be used out of the box to a chart.js module that expects object literals as both config and data because the JS to Py and back got lost in translation as data.

We had to use 2 workarounds there:

export data and options as JSON then pass these as strings then provide an utility on the main thread that would instantiate a Chart by parsing that JSON back ... it works and it's super ugly and definitively not efficient
use a copy/paste to_js that does the right transformation out of the box via dict_converter and spend time to explain why a JS thing got converted one way and broke the other way back

None of this would've been needed if LiteralMap as proposed would've been used instead and, most importantly, no users' expectations would've required me explaining that almost everyone using JS APIs has the same issue and we're trying to work on it in the best possible way.

I am telling this simply because I see this is still a Draft and nothing happened since my latest comment ... but we really would like for this to happen, in a way or another, thank you 🙏

hoodmane · 2024-03-07T18:29:02Z

I am willing to add tests and merge this as long as map.get always refers to the map prototype method and not an object value. That would make this entirely backwards compatible. If not, we need an extra discussion about breaking the previous behavior of toJs for dictionaries with "get" as a key. @ryanking13 wdyt?

WebReflection · 2024-03-07T19:13:14Z

@hoodmane I am OK doing that and deal with edge cases such as JS descriptors with an explicit get field that should point at that explicit get field.

I don't think this is the right decision though, but it's definitively better than the current state as we just keep having reported on our side the same issue over and over ... I won't port that to my module because there won't ever be a way to retrieve an explicit {get: 123} literal from JS and have it back as it was from any API expecting or consuming that object as it is (including Object.defineProperty(ref, 'accessor', {get() { return 123 }}) ... those cases will need an explanation from our side but hey, I guess we can't have the cake and eat it too 😁

rth · 2024-03-07T20:25:45Z

I think it's a good idea overall, but I don't have anything relevant to say on the details of the implementation.

The .js file should at least have clear attribution if you decide not to have that library as a dependency. You were probably going to do that anyway I imagine.

Thanks for working on this subject!

ryanking13 · 2024-03-08T00:46:02Z

I am willing to add tests and merge this as long as map.get always refers to the map prototype method and not an object value. That would make this entirely backwards compatible. If not, we need an extra discussion about breaking the previous behavior of toJs for dictionaries with "get" as a key. @ryanking13 wdyt?

I think we all agree on the need for LiteralMap, so I think we can merge it in a backward-compatible way for now and discuss the get descriptor separately.

When it comes to the behavior of get descriptors, I can see both sides of the argument, and I don't think there is a 100% right answer. However, if backward incompatible changes are an issue, I think it's better to make the necessary changes quickly.

In any case, I think issues with get descriptors will be rare, and the introduction of LiteralMap solves a huge pain point in itself, so I'm very excited about having this change.

ntoll · 2024-03-08T11:35:09Z

@hoodmane hey hey matey!

I'm confused (as usual - bear with me). 😉

I am willing to add tests and merge this as long as map.get always refers to the map prototype method and not an object value. That would make this entirely backwards compatible. If not, we need an extra discussion about breaking the previous behavior of toJs for dictionaries with "get" as a key.

Surely the behaviour of .get depends on the intention around how the underlying JS object is used? I agree with you that map.get should definitely be the default behaviour. Who wouldn't? However, if someone has an object with a get key, then on their head be it, right..? Their intention is that get refers to whatever value it is they've assigned to the get key, and that's their decision (and responsibility for resulting strange behaviour) because to them it makes sense in the context of their application.

It feels strange to disallow this behaviour since we're taking such a decision away from the coder. Ergo (unless I'm missing some subtle technical reason - which is likely), I don't think it breaks previous behaviour unless the user is brave/daft enough to override get - but that's their responsibility, not ours. It goes without saying that should we go with this behaviour, it should be red-flagged in the docs, and yes, we're allowing them to shoot themselves in the foot. But my intuition is folks who override get probably are lacking in the experience/context to understand the implementation details of Pyodide, or they simply don't care (and so will be confused by the solution of always referring to map.get, as it currently stands).

I'm concerned our "by fiat" decision about get is only going to limit folks and create confusion for those who are not au fait with the workings of a JavaScript Map and how they relate to Pyodide (those who are is probably a set of humans limited to the set of participants in this discussion). 🤣

Of course, and as always, just remember I have a very small brain and likely miss some subtlety in the technical situation, in which case (and only if you have time), I'd love to know more and explore this further. Put simply, I'm not sure this is a technical problem to solve, but a user defined reality of their code, for them to experience... (to misquote Kierkegaard). 😛

WebReflection · 2024-03-08T12:36:17Z

In any case, I think issues with get descriptors will be rare ...
I don't think it breaks previous behaviour unless the user is brave/daft enough to override get ...

I agree with both points. My only one is that with current LiteralMap as intended by my module there is an explicit escape hook to be sure the Map get is used, but that's not necessarily reflected in Python code from the JS module, so I see Hood argument too.

Mine is that whoever really needs or expect a Map in the JS side and is paranoid enough about the rare get case can also workaround that via an explicit Map.prototype.get.call(proxy, key), while returning always that method instead makes get inaccessible by all means, hence lost in translation and intents.

I think we can merge it in a backward-compatible way for now and discuss the get descriptor separately.

This would be wonderful because it will eventually expose the issue, if ever encountered, unlocking the current state.

Last, but not least, I don't care much about attribution if it's not exactly my module, some "thanks to the PyScript team for the idea" in the release notes would be surely appreciated but also not strictly necessary if you don't think that should be there ... it's OK, we just can't wait to stop explaining to our users that when a dictionary is passed to any JS API that's a Map and a Map only by default.

Thank you 🙏

ryanking13

Thanks for a productive discussion, everyone.

I'll approve this so this can go forward. Codewise looks good to me. Thanks to @WebReflection again for implementing LiteralMap.

Let's merge this as-is and revisit get descriptor issue when we actually encounter such issues from the users. If this issue is being raised frequently by multiple users, there's a good reason to reconsider it, and I think we'll get more real-world feedback from users who have actually experienced the issue.

ntoll · 2024-03-15T17:00:24Z

Hurrah, thank you everyone for the work done on this. Lots of people looking forward to this landing..! 🎉

WebReflection · 2024-03-15T21:47:24Z

@ryanking13 thank you a lot for your final thoughts!

Some thing to maybe think about later on:

practical ... this is still a Draft so it can't get merged 😅
techinal ... there are conflicts but it looks like none of them would affect this decision 🚀
informal ... the MicroPython runtime just decided to keep interpreter.globals.get(...) working as it used to, but every other Python dictionary is going to be translated by default as JS object literal and not Map. We're super happy about that decision but it should be clear in here that this is not what Pyodide will do, here we are just providing a "magic default converter" that would make many people happy, yet that is not what the underlying Proxy does so that such conversion is needed at all. This point is just to bring up that while I am super happy LiteralMap got a chance in this project, other runtimes actually understood that it's pretty natural and normal to provide destructuring and object literal expectations from any JS API written to date, from Web standards to Server related one, and that the need of the conversion is something always awkward to explain to our users ... they won't need that anymore in MicroPthon case, they need to understand why Pyodide needs such conversion when a JS signature/function/API is used ... check your fetch(...) workaround yourself, and realize when in JS land, that conversion should always be the default. Now, I am not here to judge this project or the wonderful progress this MR would make, I am just hoping you all will, eventually, realize, the LiteralMap primitive/proxy-traps are actually what any Python dictionary should look like when it comes to the JS world, in all occasion, or better, by default, without any API or FFI around!

Again, I can't wait wait to see this landing, but I felt like worth it to "plant the seed" about what LiteralMap traps allow to this, and others, project to achieve, in the name of UX.

You all have a lovely weekend 👋

ryanking13 · 2024-03-19T14:14:20Z

this is still a Draft so it can't get merged

Yeah, I think this PR still needs documentation updates and some more tests if possible. My approval was to express that I agree with this PR overall, so let's not let it stuck for long (and let's land this at least before the 0.26.0 release :) ).

but I felt like worth it to "plant the seed" about what LiteralMap

Yes, I like the phrase "planting the seed". Someday we may change the type translation behavior so that the Dict=>Object conversion should always be the default (or maybe not). However, I think it's very positive to be able to experiment with it in a backward-compatible way before making the big changes.

hoodmane · 2024-03-19T15:11:18Z

There are a lot of dictionaries that have non-string keys, and converting dictionaries to Objects does not have a good way to convert them because an Object can only have a string (or symbol) as a key. I don't think we're likely to get the ffi much better than it is by having a completely generic rule that works well for everything. (Though it would be good to add a JavaScript equivalent of as_object_map.)
I think the better path here is to attach "signatures" to the JavaScript functions involved indicating intent, e.g.,:

class SomeOtherThing(TypedDictCopyToObject):
   key1: str
   key2: int
   

class GlobalThis:
   @staticmethod
   def fetch(url, *, method: str, headers: StringDictCopyToObject[str], some_other_thing: SomeOtherThing):
      pass

import js
sys.modules["js_convenient_to_use"] = js.bind_ffi_signature(GlobalThis)

This should also give us type hints at the same time. I've started prototyping a system like this, hopefully I'll be able to get something working in the next few months.

WebReflection · 2024-03-19T16:14:45Z

@hoodmane I honestly didn't fully follow your concern but nobody in JS APIs expect Python things as keys and, most importantly, both integers and strings are handled exactly the same from a Proxy point of view, even with proxied Arrays in the JS world itself, so I am not sure that example is representative of the issue you are trying to solve.

To clarify, in JS obj[1] or obj["1"] are handled exactly the same, reason I did implement your filter suggestion, but now that I read your examples/concerns I am not fully sure I should've ...

const array = ref => new Proxy(ref, {
    get(ref, key) {
        console.log(typeof key);
        // it's always "string" / "symbol"
        // in JS proxies everything to get is a string/symbol
        return ref[key];
    }
});

const proxy = array([1, 2, 3]);
const result = proxy[0];

console.assert(result === 1);
// no Assertion failure here, it's 1

edit bear in mind this is true for every single Proxy trap so I understand why ownKeys had a special treatment but I would really like to understand which scenario between Python and JS any of this would be problematic, thank you!

ryanking13 · 2024-03-21T15:17:03Z

There are a lot of dictionaries that have non-string keys, and converting dictionaries to Objects does not have a good way to convert them because an Object can only have a string (or symbol) as a key.

Yes, I agree that there are challenges to directly converting Python dictionaries to JS objects, including non-string keys. And I think that's why we're introducing LiteralMap. We still use JS Maps to hold non-string keys from Python dictionaries, but we're providing support to use Python dictionaries as JS Object without extra conversion as well.

hoodmane · 2024-03-21T16:52:17Z

we're providing support to use Python dictionaries as JS Object without extra conversion as well

I don't think this PR does this, it only changes the behavior of toPy not of a PyProxy.

WebReflection · 2024-03-21T20:02:55Z

@hoodmane that’s already a starting point to me … give you time to realize if that’s welcomed and desired, then think bigger (make it part of your default PyProxy logic) imho … or go big all at once, your choice

ryanking13 · 2024-03-22T03:55:07Z

I don't think this PR does this, it only changes the behavior of toPy not of a PyProxy.

Oh, yeah. That's what I meant.

ntoll · 2024-03-22T14:11:41Z

Hi Folks,

Good to see discussion going on here...

I don't think we're likely to get the ffi much better than it is by having a completely generic rule that works well for everything

Tech is all about compromises. The trick is knowing which compromises to make. 😉

So, in this spirit of of trying to figure out the right way forward... the work in the PR is a great first step, and can we perhaps ask folks in our wider communities to reflect and feed back on their unique situations so at least we have a good impression of what either Pyodide or PyScript "do" given certain situations as we move forward...? I can't help but feel we're flying a bit blind without such context and we collectively are at our best when we're open and share such information. We lift each other up, and our worlds mutually grow (which I always appreciate).

Speaking personally, there's not a week goes by where I have to explain Map and Object to a bemused PyScript user (and my gut feeling is this burden is likely to increase). Anything we can do to help these folks so things "just work" ™️ most of the time (and the land mines we inevitably compromise over are well labelled in all our docs... just so we all have our stories lined up and coherent), would be welcomed by many and help to solve a LOT of support related explanations when folk do some variation of:

import js

js.some_module_or_other(config={
  "foo": "bar",
})

...and it simply doesn't work.

We've got this... and I think it'd be wonderful to chat about ways-of-working in this regard at the web assembly summit at PyCon. I'm looking forward to catching up over dinner! 🍻

Have a great weekend folks! 👍 🤗

ryanking13 · 2024-03-23T10:06:08Z

So... This PR doesn't affect the behavior of PyProxy itself, but I think it's good enough that it changes the behavior of converting PyProxy to Map with toJs() anyway. At least, we don't need to tell people to use dict_converter anymore when using toJs.

attach "signatures" to the JavaScript functions involved indicating intent

I am very interested in your idea, but maybe it can go separate from this PR? I mean, I think there are two things we're talking about right now:

Changing PyProxy.toJs() of Python dictionary to be LiteralMap instead of just Map

This is what this PR does. So people will still need to call toJs before using python dictionary in JS world to use it like a JS object, but it removes extra JS Map ==> JS Object conversion.

Making PyProxy of Python dictionary work like a JS Object

This is what people mostly expect, so that

js.some_module_or_other(config={
  "foo": "bar",
})

just work.

So I am open to further discussion on (2), either in another issue or at the PyCon, but I think this PR is sufficient for (1), what do you think?

hoodmane · 2024-03-23T11:40:40Z

but maybe it can go separate from this PR?

Definitely, this pr just needs tests and is ready to go.

for more information, see https://pre-commit.ci

tedpatrick · 2024-03-25T22:37:44Z

🚀🚀🚀 Awesome! Go @hoodmane Go! 🚀🚀🚀

ntoll · 2024-03-26T09:09:57Z

Hurrah!

Make to_js return a LiteralMap

0c3c5db

This acts like both a Map and an Object. Co-authored-by: Andrea Giammarchi <andrea.giammarchi@gmail.com>

hoodmane force-pushed the literal-map branch from 5aec054 to 0c3c5db Compare February 28, 2024 02:56

hoodmane marked this pull request as draft February 28, 2024 02:56

hoodmane mentioned this pull request Feb 28, 2024

Provide a loadPyodide({defaultDictConverter: Object.fromEntries}) field. #4030

Closed

Fix tests

6cd2501

WebReflection reviewed Feb 28, 2024

View reviewed changes

WebReflection reviewed Mar 1, 2024

View reviewed changes

ryanking13 reviewed Mar 2, 2024

View reviewed changes

hoodmane added this to the 0.26.0 milestone Mar 8, 2024

ryanking13 approved these changes Mar 15, 2024

View reviewed changes

Merge branch 'main' into HEAD

195263b

Add tests

b0d15ca

hoodmane marked this pull request as ready for review March 25, 2024 11:05

pre-commit-ci bot and others added 2 commits March 25, 2024 11:06

[pre-commit.ci] auto fixes from pre-commit.com hooks

ec97f7f

for more information, see https://pre-commit.ci

Update changelog

dd7bca4

hoodmane merged commit a71970e into pyodide:main Mar 25, 2024

ryanking13 deleted the literal-map branch March 26, 2024 07:33

hoodmane mentioned this pull request Apr 24, 2025

Python Workers: Support importable env cloudflare/workerd#3988

Merged

Uh oh!

Make to_js of a dictionary return a LiteralMap #4576

Make to_js of a dictionary return a LiteralMap #4576

Uh oh!

Conversation

hoodmane commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WebReflection Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

WebReflection Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoodmane Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebReflection Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

WebReflection Mar 1, 2024

Choose a reason for hiding this comment

Uh oh!

WebReflection Mar 1, 2024

Choose a reason for hiding this comment

Uh oh!

WebReflection Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

WebReflection commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoodmane commented Mar 7, 2024

Uh oh!

WebReflection commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rth commented Mar 7, 2024

Uh oh!

ryanking13 commented Mar 8, 2024

Uh oh!

ntoll commented Mar 8, 2024

Uh oh!

WebReflection commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

ntoll commented Mar 15, 2024

Uh oh!

WebReflection commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanking13 commented Mar 19, 2024

Uh oh!

hoodmane commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WebReflection commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanking13 commented Mar 21, 2024

Uh oh!

hoodmane commented Mar 21, 2024

Uh oh!

WebReflection commented Mar 21, 2024

Uh oh!

ryanking13 commented Mar 22, 2024

Uh oh!

ntoll commented Mar 22, 2024

Uh oh!

ryanking13 commented Mar 23, 2024

Uh oh!

hoodmane commented Mar 23, 2024

Make `to_js` of a dictionary return a `LiteralMap` #4576

Make `to_js` of a dictionary return a `LiteralMap` #4576

hoodmane commented Feb 28, 2024 •

edited

Loading

WebReflection Feb 28, 2024 •

edited

Loading

hoodmane Feb 28, 2024 •

edited

Loading

WebReflection commented Mar 7, 2024 •

edited

Loading

WebReflection commented Mar 7, 2024 •

edited

Loading

WebReflection commented Mar 8, 2024 •

edited

Loading

WebReflection commented Mar 15, 2024 •

edited

Loading

hoodmane commented Mar 19, 2024 •

edited

Loading

WebReflection commented Mar 19, 2024 •

edited

Loading