Fix #167: Add Fetch API + Streams API #177

bblfish · 2015-11-25T14:55:01Z

I have been using the Fetch API for my rww-scala-js akka branch. The code that I have tested and used works. So I thought I'd push this over before someone goes through the same work again.

Probably still a lot to be improved.

This would close #167

bblfish · 2015-11-25T14:56:24Z

note: I am quite sure there will be improvements needed to the formatting. But before going through all that, I thought I'd first check if this is of interest.

Btw. is there a script to get the formatting just right for this repo?

bblfish · 2015-11-25T15:29:23Z

src/main/scala/org/scalajs/dom/experimental/JSIterable.scala

+  * @tparam A
+  */
+@js.native
+trait JSIterable[A] extends js.Object {


not sure actually where this should go. But it has proven useful in my code to be able to iterate through the headers.

bblfish · 2015-11-25T20:12:36Z

Not sure what the travis failures are related to.

sjrd · 2015-11-25T21:57:46Z

[error] /home/travis/build/scala-js/scala-js-dom/src/main/scala/org/scalajs/dom/experimental/package.scala:12:
  Classes, traits and objects inheriting from js.Any should be annotated with @js.native,
  unless they have @ScalaJSDefined. The default will switch to Scala.js-defined
  in the next major version of Scala.js.
[error] package object experimental extends js.GlobalScope {
[error]                ^
[error] one error found
[error] (root/compile:compile) Compilation failed

bblfish · 2015-11-25T22:08:02Z

yes but when I add @js.native then I get

> compile
[info] Compiling 6 Scala sources to /Volumes/Dev/Programming/ScalaJS/scala-js-dom/target/scala-2.11/classes...
[error] /Volumes/Dev/Programming/ScalaJS/scala-js-dom/src/main/scala/org/scalajs/dom/experimental/package.scala:13: expected start of definition
[error] package object experimental extends js.GlobalScope {
[error] ^
[error] one error found

what I don't understand is that my own code seems to compile.

sjrd · 2015-11-25T22:10:07Z

Yep, native package objects are sort of deprecated (scala-js/scala-js#1892). This build fails because we have -Xfatal-warnings.

bblfish · 2015-11-25T22:13:11Z

What do you recommend I do instead?

It looks like the documentation on facade-types needs to be changed. It allows package object without the @js.native

sjrd · 2015-11-25T22:24:16Z

Yes, we should change it.

Use a named object, like @js.native object Fetch extends js.GlobalScope.

sjrd · 2015-11-25T22:24:50Z

If you absolutely want the fetch method visible at the top-level of the package (which I'm sure is a good idea), you can write a Scala forwarder.

bblfish · 2015-11-25T22:59:45Z

Ok, it compiles now. I did not add fetch to the package object because I am not sure how useful that is and how much thought I had put into that.

bblfish · 2015-11-25T23:25:06Z

src/main/scala/org/scalajs/dom/experimental/Stream.scala

+  /**
+    * The locked getter returns whether or not the readable stream is locked to a reader.
+    * throws scala.scalajs.js.TypeError  if the stream is not readable
+    */


It would not publishLocal with @throws

bblfish · 2015-11-27T14:03:07Z

I tried to get as close to the preferred formatting of this project in the last commit. I also took out some classes related to streaming to a separate file. I worked on this before my previous commits, so the code may not be as good as my last one. But it is quite a lot of code.

sjrd · 2015-11-27T14:30:35Z

I don't have the bandwidth to process this these days. Could someone else review this? @mseddon? @nicolasstucki?

bblfish · 2015-11-27T15:36:51Z

src/main/scala/org/scalajs/dom/experimental/JSIterable.scala

+trait JSIterator[+A] extends js.Object {
+  def next(): JSIterator.Entry[A] = js.native
+}
+


I suppose this one is for @sjrd . I added this here as there is none available yet in the standard library. Is that ok?
See scala-js/scala-js#1141

It would be better to submit this as a PR to scala-js/scala-js.

Ok, I'll do that too. Is there a package in which it should be placed there?

mseddon · 2015-11-28T11:58:43Z

@sjrd @bblfish I have a bit of free time tomorrow, so I can pick this up from then.

mseddon · 2015-11-29T15:19:00Z

src/main/scala/org/scalajs/dom/experimental/Fetch.scala

+ *
+ * bblfish: not clear from the definition if this is a number of traits that
+ * may be implemented by the Response or not
+ */


Unclear what you mean by this- certainly body is a trait that is implemented by Response with this signature.

sjrd · 2015-12-02T19:39:13Z

src/main/scala/org/scalajs/dom/experimental/Stream.scala

+   * method is intentionally generic; it does not require that its this
+   * value be a ReadableStream object.
+   */
+  def tee(): js.Array[ReadableStream[T]] = js.native


The doc mentions a "given writable" stream", but the signature doesn't. That's suspicious.

bblfish · 2015-12-03T18:05:45Z

I fixed a few of the points with JSIterable and JSIterator, but quite a few of the methods do need a js.native, in order to access the underlying JS functions. I have tested it with my application read-write-web/rww-scala-js@ebb8b7e

sjrd · 2015-12-03T18:24:36Z

but quite a few of the methods do need a js.native, in order to access the underlying JS functions.

What makes you think that?

bblfish · 2015-12-03T18:29:33Z

src/main/scala/org/scalajs/dom/experimental/JSIterable.scala

+  trait Entry[+A] extends js.Object {
+    def done: Boolean = js.native
+    def value: A = js.native
+  }


value and done are two js.native functions, so Entry can't be @ScalaJSDefined right?

perhaps not. I suppose it's such a basic class...

bblfish · 2015-12-03T18:58:37Z

Is that better?

sjrd · 2015-12-03T21:15:54Z

src/main/scala/org/scalajs/dom/experimental/Fetch.scala

+
+  def integrity: js.UndefOr[String]
+
+  def window: js.UndefOr[Null]


js.UndefOr[Null] looks suspicious ^^

yes, but it's actually what they seem to say in the docs. "can only be null" .
That's apparently to allow you to use it like ServiceWorkers from a window.
The idea is you can say you don't want to have a window. Don't really understand it.

Lol. OK, then.

bblfish · 2015-12-04T09:38:44Z

No more comments left in the code tab. Ready to go?

sjrd · 2015-12-04T09:48:50Z

src/main/scala/org/scalajs/dom/experimental/Stream.scala

+   * branches. (Let us know if you think we should add an option to tee that
+   * creates structured clones of the chunks for each branch.)
+   */
+  def tee(): js.Array[ReadableStream[T]] = js.native


Ah I see this was blocking the covariant thing.
Would js.Array[_ <: ReadableStream[T]] allow you to make T covariant?

Ideally this should even be a js.Tuple2[ReadableStream[T], ReadableStream[T]], which would also solve the covariant issue.

bblfish · 2015-12-04T11:13:39Z

done.

sjrd · 2015-12-04T12:29:07Z

Good!

Last step: could you squash all the commits, please?

bblfish · 2015-12-04T12:57:58Z

commits squashed. I learned a lot with the PR. :-)

mseddon · 2015-12-04T12:59:08Z

👍 whew! Thanks for that @bblfish and @sjrd, good work!

sjrd · 2015-12-04T13:16:26Z

LGTM, thanks!

Fix #167: Add Fetch API + Streams API

bblfish reviewed Nov 25, 2015
View reviewed changes

bblfish force-pushed the FetchAPI branch from 3bd8c29 to 856e88f Compare November 27, 2015 13:44

bblfish reviewed Nov 27, 2015
View reviewed changes

mseddon mentioned this pull request Nov 28, 2015

Documentation shows package object extending js.GlobalScope without @js.native scala-js/scala-js-website#203

Closed

bblfish added a commit to read-write-web/rww-scala-js that referenced this pull request Nov 28, 2015

move to org.scalajs.dom scala-js/scala-js-dom#177

f2942b6

mseddon reviewed Nov 29, 2015
View reviewed changes

sjrd reviewed Dec 2, 2015
View reviewed changes

bblfish reviewed Dec 3, 2015
View reviewed changes

sjrd reviewed Dec 3, 2015
View reviewed changes

sjrd reviewed Dec 4, 2015
View reviewed changes

Fix scala-js#167: Add Fetch API + Streams API

12658a4

bblfish force-pushed the FetchAPI branch from fba82b7 to 12658a4 Compare December 4, 2015 12:50

bblfish changed the title ~~add Fetch API~~ Fix #167: Add Fetch API + Streams API Dec 4, 2015

sjrd added a commit that referenced this pull request Dec 4, 2015

Merge pull request #177 from bblfish/FetchAPI

dad18b7

Fix #167: Add Fetch API + Streams API

sjrd merged commit dad18b7 into scala-js:master Dec 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #167: Add Fetch API + Streams API #177

Fix #167: Add Fetch API + Streams API #177

bblfish commented Nov 25, 2015

bblfish commented Nov 25, 2015

bblfish Nov 25, 2015

bblfish commented Nov 25, 2015

sjrd commented Nov 25, 2015

bblfish commented Nov 25, 2015

sjrd commented Nov 25, 2015

bblfish commented Nov 25, 2015

sjrd commented Nov 25, 2015

sjrd commented Nov 25, 2015

bblfish commented Nov 25, 2015

bblfish Nov 25, 2015

bblfish commented Nov 27, 2015

sjrd commented Nov 27, 2015

bblfish Nov 27, 2015

sjrd Nov 27, 2015

bblfish Nov 27, 2015

mseddon commented Nov 28, 2015

mseddon Nov 29, 2015

sjrd Dec 2, 2015

bblfish commented Dec 3, 2015

sjrd commented Dec 3, 2015

bblfish Dec 3, 2015

bblfish Dec 3, 2015

bblfish commented Dec 3, 2015

sjrd Dec 3, 2015

bblfish Dec 3, 2015

sjrd Dec 3, 2015

bblfish commented Dec 4, 2015

sjrd Dec 4, 2015

bblfish commented Dec 4, 2015

sjrd commented Dec 4, 2015

bblfish commented Dec 4, 2015

mseddon commented Dec 4, 2015

sjrd commented Dec 4, 2015


		def integrity: js.UndefOr[String]

		def window: js.UndefOr[Null]

Fix #167: Add Fetch API + Streams API #177

Fix #167: Add Fetch API + Streams API #177

Conversation

bblfish commented Nov 25, 2015

bblfish commented Nov 25, 2015

Choose a reason for hiding this comment

bblfish commented Nov 25, 2015

sjrd commented Nov 25, 2015

bblfish commented Nov 25, 2015

sjrd commented Nov 25, 2015

bblfish commented Nov 25, 2015

sjrd commented Nov 25, 2015

sjrd commented Nov 25, 2015

bblfish commented Nov 25, 2015

Choose a reason for hiding this comment

bblfish commented Nov 27, 2015

sjrd commented Nov 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseddon commented Nov 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblfish commented Dec 3, 2015

sjrd commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblfish commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblfish commented Dec 4, 2015

Choose a reason for hiding this comment

bblfish commented Dec 4, 2015

sjrd commented Dec 4, 2015

bblfish commented Dec 4, 2015

mseddon commented Dec 4, 2015

sjrd commented Dec 4, 2015