Skip to content

Conversation

@dom96
Copy link
Contributor

@dom96 dom96 commented Nov 12, 2024

The big change here is the addition of the Blob API. But there are also some other improvements/fixes:

  • Each class instance now exposes a js_object property which returns its underlying JS object.
  • The FormData API is now compatible with the multidict package's API
  • The FormData API can now accept Blobs and returns the Python Blobs too

Test Plan

$ bazel run @workerd//src/workerd/server/tests/python:sdk/sdk

@dom96 dom96 marked this pull request as ready for review November 12, 2024 15:27
@dom96 dom96 requested review from a team as code owners November 12, 2024 15:27
Comment on lines +30 to +31
# TODO: Pyodide's FetchResponse.headers returns a dict[str, str] which means
# duplicates are lost, we should fix that so it returns a http.client.HTTPMessage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still think we should do this? I thought the duplicate values are combined with a comma. How does http.client.HTTPMessage handle values with a comma in them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not super important, but it would be nice. Splitting values by comma is pretty bleh as far as API design.

But I guess on the other hand that is what JS' Headers expects so maybe it's fine.

@dom96 dom96 force-pushed the dominik/blob-api branch 3 times, most recently from 4381834 to 46f1485 Compare November 13, 2024 10:52
Comment on lines 300 to 305
**options: Unpack[BlobKwargs],
):
if "content_type" in options:
options["type"] = options["content_type"]
if "endings" in options:
options["endings"] = str(options["endings"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it might be better to be explicit here:

Suggested change
**options: Unpack[BlobKwargs],
):
if "content_type" in options:
options["type"] = options["content_type"]
if "endings" in options:
options["endings"] = str(options["endings"])
content_type: str | None = None,
endings: BlobEnding | str | None = None
):
if endings:
endings = str(endings)

and then:

    js.Blob.new(args, type=content_type, endings=endings)

Also, I am pretty sure that you don't need to_js for args.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I am pretty sure that you don't need to_js for args.

You do, I get this otherwise: pyodide.ffi.JsException: TypeError: Failed to construct 'Blob': constructor parameter 1 is not of type 'Array'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate. There's been discussion before whether we should make Array.isArray() return true on a Python list. Not sure whether it would fix this error message. I also don't remember why I didn't do it -- it's possible I just never got to it, or that there was some problem it caused.

Copy link
Contributor

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good now, thanks @dom96. I have one last remark.

@dom96 dom96 force-pushed the dominik/blob-api branch 2 times, most recently from 0dec886 to 548b5f1 Compare November 14, 2024 14:22
@dom96 dom96 requested a review from a team November 14, 2024 14:22
@dom96 dom96 merged commit 186655e into main Nov 14, 2024
@dom96 dom96 deleted the dominik/blob-api branch November 14, 2024 16:49
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