-
Notifications
You must be signed in to change notification settings - Fork 162
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
Conversation
I was unable to run
It could be an issue with my environment - I am running Windows. WSL returned same errors. |
Hmm, looks like a problem with the |
Ah, of course. The docs use |
You may want to check the code in here, see if there's something you need to update: |
Yes, there is a change to |
CI does show some actual Firefox and Scala 3 errors, those I obviously can fix. |
Does |
Yes 😞 |
Huh, with that same error message? Now that's definitely strange :) |
tests-shared/src/main/scala/org/scalajs/dom/tests/shared/SharedTests.scala
Outdated
Show resolved
Hide resolved
tests-shared/src/main/scala/org/scalajs/dom/tests/shared/SharedTests.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamController.scala
Outdated
Show resolved
Hide resolved
I think the only issue left now is the very first one - #628 (comment). |
And it was a Windows related issue. Big thanks to Microsoft for WSL 👏 |
Nice work getting it green! 🎉 |
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.
This is a very complex API, so thank you for all your efforts and apologies if I'm confused about something in the comments!
dom/src/main/scala/org/scalajs/dom/ReadableStreamQueuingStrategy.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamController.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamQueuingStrategy.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamQueuingStrategy.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamUnderlyingSource.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamUnderlyingSource.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamUnderlyingSource.scala
Outdated
Show resolved
Hide resolved
tests-shared/src/main/scala/org/scalajs/dom/tests/shared/BrowserTests.scala
Show resolved
Hide resolved
@armanbilge Thank you for the review! I think I have addressed all of your comments. |
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.
tests-shared/src/main/scala/org/scalajs/dom/tests/shared/BrowserTests.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamController.scala
Outdated
Show resolved
Hide resolved
dom/src/main/scala/org/scalajs/dom/ReadableStreamUnderlyingSource.scala
Outdated
Show resolved
Hide resolved
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.
This looks great, thanks for all your efforts! Just one last tiny thing 😅
tests-shared/src/main/scala/org/scalajs/dom/tests/shared/BrowserTests.scala
Show resolved
Hide resolved
Co-authored-by: Arman Bilge <[email protected]>
Co-authored-by: Sébastien Doeraene <[email protected]>
@ptrdom thanks again for this fantastic contribution! It is now published as |
Hello! Do you plan to cut a release with these changes soon? |
@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. 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. |
It is not just about the change to the Fetch facade, the PR contains other changes related to the 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 |
Ah, that's right, you want to use this as your stream implementation :) thanks for the reply. I'll start getting a release together. |
Thank you! |
References #622.