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

moz-chunked-arraybuffer vs. moz-chunked-text #13

Closed
ariutta opened this issue Aug 11, 2016 · 12 comments
Closed

moz-chunked-arraybuffer vs. moz-chunked-text #13

ariutta opened this issue Aug 11, 2016 · 12 comments

Comments

@ariutta
Copy link
Contributor

ariutta commented Aug 11, 2016

I was wondering why the responseType is moz-chunked-arraybuffer here instead of moz-chunked-text. The array buffers are always converted to text (here and here). Are you are planning to use array buffers in a future version of this library? Or are array buffers better for communication over the wire?

@jonnyreeves
Copy link
Owner

jonnyreeves commented Aug 11, 2016

I was wondering why the responseType is moz-chunked-arraybuffer here instead of moz-chunked-text.

I can't recall a strong design choice behind that one; however it does provide consistency with the ReadableByteStream approach.

Are you are planning to use array buffers in a future version of this library?

#12 (implemetning streaming binary protocols) would be implemented by pushing the 'to-text' decoding part into the (default) chunk parser.

Are array buffers better for communication over the wire?

It's not something I've benchmarked; might be a bit tricky to do so in a way that's not also benchmarking an underlying HTTP server (even when running locally) - would also be interested to see some concrete data :)

@ariutta
Copy link
Contributor Author

ariutta commented Aug 11, 2016

pushing the 'to-text' decoding part into the (default) chunk parser.

For my users, I can't ignore browsers without TextDecoder yet, so I'll need to support text responses in addition to ArrayBuffers for awhile. Perhaps this library could feed the parser an ArrayBuffer when TextDecoder is defined in the environment but otherwise return text. Each parser would need a section something like this:

if (root.TextDecoder) {
  // parse ArrayBuffer 
} else {
  // parse text
}

concrete data

Agreed!

@ariutta
Copy link
Contributor Author

ariutta commented Aug 11, 2016

Instead of forcing parsers to handle both ArrayBuffers and text, another option would be to use the polyfill for the Encoding Living Standard's API. It's really large at 537K minified, so maybe we could get away with just the UTF8 version, which is 5.4K minified.

@ariutta
Copy link
Contributor Author

ariutta commented Aug 11, 2016

It appears Safari and IE10+ support ArrayBuffers but not TextDecoder. How hard would it be for the parsers to handle ArrayBuffers without guaranteed availability of TextDecoder?

@ariutta
Copy link
Contributor Author

ariutta commented Aug 12, 2016

How about this: the input to each parser is always a reader equivalent to the result of calling getReader for WHATWG streams and fetch, e.g.:

fetch('/big-data.csv').then(function(response) {
  var reader = response.body.getReader();
  ...
});

This reader has a read method that returns a Promise that is fulfilled when the next data chunk arrives. Each data chunk is an ArrayBuffer.

For browsers that support fetch, this API would mean simply passing the reader to the parser, but for other browsers, we'd need to create a polyfill for reader. This polyfill would use XHR to get a stream of ArrayBuffers, and a reader.read call would return a Promise with the latest unread chunk(s) from the stream.

To get a stream of ArrayBuffers would require some browser-specific processing. For FF and IE10+, we'd just set XHR responseType to moz-chunked-arraybuffer or ms-stream. For Safari and IE9, it's not possible to get a stream, so we'd need to read in the entire response and emit chunks by keeping a running index and slicing off chunks from the accumulating response. For Safari, we can set responseType to arraybuffer, but for IE9 (if it's worth supporting), we'd need to set the responseType to text and convert each chunk to an ArrayBuffer using TextEncoder from the UTF8 polyfill for the Encoding Living Standard's API.

Does that sound reasonable?

@ariutta
Copy link
Contributor Author

ariutta commented Aug 12, 2016

Actually for IE9, we could use something simpler like this to encode text to an ArrayBuffer, because WHATWG says the encoder must use UTF8:

function decode(buf) {
  return String.fromCharCode.apply(null, new Uint8Array(buf));
}
function encode(str) {
  var buf = new ArrayBuffer(str.length);
  var bufView = new Uint8Array(buf);
  for (var i=0, strLen=str.length; i < strLen; i++) {
    bufView[i] = str.charCodeAt(i);
  }
  return buf;
}

(modified from here)

Supporting IE9 would also require a polyfill for Uint8Array, maybe from https://github.com/substack/typedarray.

@jonnyreeves
Copy link
Owner

Thanks for the thought and time you have put into this @ariutta!

the input to each parser is always a reader equivalent ... Each data chunk is an ArrayBuffer.

This sounds like an excellent suggestion; in the future we could always allow the caller to specify if they want to get bytes or text fed from the reader (although at present I will only be looking at a Byte stream).

Supporting IE9...

It's not something I'm interested in doing (my target use-case only covers evergreen browsers) - hover I would be happy to add support in the future - library filesize is not really one of my primary drivers for this lib.

Thanks again!

@ariutta
Copy link
Contributor Author

ariutta commented Aug 17, 2016

Thanks for getting this library started, @jonnyreeves!

For the parsers, the library jBinary looks interesting: "High-level API for working with binary data."

@ariutta
Copy link
Contributor Author

ariutta commented Aug 18, 2016

I've started implementing these ideas here:
https://github.com/ariutta/chunked-request

It's not at all production-ready yet, but feel free to take a look! And you can even start integrating any or all of my code if you like, but just be warned that it needs much more testing.

You'll see lots of TODOs in the code, which might be useful places for you to check out and provide feedback.

@ariutta
Copy link
Contributor Author

ariutta commented Aug 18, 2016

I didn't change the API yet to pass a reader to the parser, but I still think that's the best option when either of us has a chance to implement it.

@jonnyreeves
Copy link
Owner

@ariutta hahah; I was hacking on this last night as well :D I've raised my pull request as #15, I would be greatful if you could give it a review, although it appears to share the same approach as you were going for.

@ariutta
Copy link
Contributor Author

ariutta commented Aug 19, 2016

Looks good! Commented on the pull request.

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

No branches or pull requests

2 participants