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

Fetch request body as ReadableStream #628

Merged
merged 13 commits into from
Nov 17, 2021

Conversation

ptrdom
Copy link
Contributor

@ptrdom ptrdom commented Nov 10, 2021

References #622.

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 10, 2021

I was unable to run sbt prePR, received this error:

[error] /scala-js-dom/readme/Index.scalatex:43:6: type mismatch;
[error]  found   : scalatex.site.Section.Proxy
[error]  required: scalatags.Text.all.Frag
[error]     (which expands to)  scalatags.generic.Frag[scalatags.text.Builder,String]
[error] @sect{scala-js-dom}
[error]      ^
[error] one error found
[error] (readme / Compile / compileIncremental) Compilation failed

It could be an issue with my environment - I am running Windows. WSL returned same errors.

@armanbilge
Copy link
Member

armanbilge commented Nov 10, 2021

Hmm, looks like a problem with the README documentation, I wonder if any of the code you changed touched that somehow. Let me try it as well.

@armanbilge
Copy link
Member

Ah, of course. The docs use Fetch which you just changed here right?
https://scala-js.github.io/scala-js-dom/#dom.Fetch

@armanbilge
Copy link
Member

You may want to check the code in here, see if there's something you need to update:
https://github.com/scala-js/scala-js-dom/blob/master/example/src/main/scala/example/Example.scala

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 10, 2021

Yes, there is a change to BodyInit type - new member added to the union. Code of the example does not interact with the request body directly, not sure what should be changed there.

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 10, 2021

CI does show some actual Firefox and Scala 3 errors, those I obviously can fix.

@armanbilge
Copy link
Member

armanbilge commented Nov 10, 2021

Does prePR fail for you before you make any changes? E.g. if you just run it on master.

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 10, 2021

Yes 😞

@armanbilge
Copy link
Member

Huh, with that same error message? Now that's definitely strange :)

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 11, 2021

I think the only issue left now is the very first one - #628 (comment).

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 11, 2021

And it was a Windows related issue. Big thanks to Microsoft for WSL 👏

@armanbilge
Copy link
Member

Nice work getting it green! 🎉

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

This is a very complex API, so thank you for all your efforts and apologies if I'm confused about something in the comments!

@ptrdom
Copy link
Contributor Author

ptrdom commented Nov 12, 2021

@armanbilge Thank you for the review! I think I have addressed all of your comments.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Sorry, just a few more things I noticed on this pass!

Let's get another pair of eyes, whenever you have a chance @sjrd or @japgolly, much appreciated! :)

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for all your efforts! Just one last tiny thing 😅

ptrdom and others added 2 commits November 14, 2021 22:58
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
Co-authored-by: Sébastien Doeraene <sjrdoeraene@gmail.com>
@armanbilge armanbilge linked an issue Nov 17, 2021 that may be closed by this pull request
@armanbilge armanbilge merged commit ed28f24 into scala-js:master Nov 17, 2021
@armanbilge
Copy link
Member

@ptrdom thanks again for this fantastic contribution! It is now published as 2.0.0+25-ed28f246-SNAPSHOT.

@julienrf
Copy link
Contributor

julienrf commented Dec 26, 2021

Hello! Do you plan to cut a release with these changes soon?

@armanbilge
Copy link
Member

@julienrf hello :) Out of curiosity, is this blocking something? Although the API facaded here is described in the spec, it's basically unsupported in browsers at this time.
https://caniuse.com/mdn-api_request_request_readablestream_request_body

Otherwise, personally I'm 👍 to a release :) Since we've added new APIs, the next release will be 2.1.0 in which we also need to fix #624. So it will take a little time to get it ready.

@julienrf
Copy link
Contributor

It is not just about the change to the Fetch facade, the PR contains other changes related to the ReadableStream API. As far as I understand, those changes are currently supported by the browsers: https://caniuse.com/?search=readablestream

This is not really blocking us since we can always define the facade we want outside of scala-js-dom, but it would be simpler to just use the one of scala-js-dom. Our related PR is endpoints4s/endpoints4s#972

@armanbilge
Copy link
Member

Ah, that's right, you want to use this as your stream implementation :) thanks for the reply. I'll start getting a release together.

@armanbilge
Copy link
Member

@julienrf I just released v2.1.0. @ptrdom thank you again for this fantastic contribution, also looks like you've been doing great work in endpoints4s :)

@julienrf
Copy link
Contributor

julienrf commented Jan 3, 2022

Thank you!

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.

Fetch request body as ReadableStream
4 participants