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

Enable "read with streaming" test in test/unit/network_spec.js #8851

Closed
Rob--W opened this issue Aug 31, 2017 · 10 comments
Closed

Enable "read with streaming" test in test/unit/network_spec.js #8851

Rob--W opened this issue Aug 31, 2017 · 10 comments
Labels

Comments

@Rob--W
Copy link
Member

Rob--W commented Aug 31, 2017

The "read with streaming" test looks like this:

it('read with streaming', function(done) {
var userAgent = window.navigator.userAgent;
// The test is valid for FF only: the XHR has support of the
// 'moz-chunked-array' response type.
// TODO enable for other browsers, e.g. when fetch/streams API is supported.
var m = /Mozilla\/5.0.*?rv:(\d+).*? Gecko/.exec(userAgent);
if (!m || m[1] < 9) {
expect(true).toEqual(true);
done();
return;
}

There are two issues here:

@yurydelendik Can the test be enabled for other browsers?

@Rob--W Rob--W added the test label Aug 31, 2017
@yurydelendik
Copy link
Contributor

Can the test be enabled for other browsers?

Yes. I don't see why not.

@aryanshar
Copy link

Hi. I would like to solve this issue. Please assign me this. And also, please guide me how do i move forward in contributing more to this repository !

@Rob--W
Copy link
Member Author

Rob--W commented Aug 31, 2017

@aryanshar That's fast, I was still typing the motivation for the good-beginner-bug label. Here it goes:

This bug consist of two parts. The first part is fully explained in the original report.

For the second part, we need to update the if condition to logic along the lines of "if (!isFirefoxWithMozChunkedEncodingSupport && !isFetchWithStreamSupport)` (choose shorter but understandable variable if you wish). The first variable name is based on the current content of the if-statement (by the way, that has a flaw too. If Firefox 90 becomes available, the condition is incorrect). The second variable should indicate whether ReadableStream is supported (see e.g. cd95b42).

@kushagra189
Copy link

Is the issue still up? I would like to solve if it is.
PS: Noob here.

@timvandermeij
Copy link
Contributor

There is a PR above, but there was no response since 24 October. You're welcome to use that PR, and in particular the comments there, as inspiration to create a new PR so we can hopefully merge it soon.

@JavierPons
Copy link

Hi there!
Do you need help for a "beginners" :p Can you explain more concretely where do you need help? I see that this issue is for @kushagra189. Do you have an other one where I can participate?

@choilmto
Copy link

Under the section titled "Browser compatibility", there is a summary of native browser support across desktop and mobile platforms. What does 'streaming response body' refer to, and is it relevant to our needs? Under whatwg's streams standard, a readable stream is a class that uses chunks...is that what line 70 in the code cited above refers to?

As an aside, instead of checking for browser support of fetch() by looking at the user-agent header for information on the browser version, can we do something like expect('fetch' in window).toBe(true);?

@Rob--W
Copy link
Member Author

Rob--W commented Dec 13, 2017

@choilmto I have updated the MDN pages to be more clear. Take another look if you want to get a better understanding.

Line 70 in the above code refers to the non-standard way of obtaining a response stream in Firefox, via the xhr.responseType = "moz-chunked-arraybuffer" (where xhr is a XMLHttpRequest instance) (so the comment has a typo, "array" should be "arraybuffer").

As an aside, instead of checking for browser support of fetch()

fetch support does not equate support for streaming responses (see the note at the end of #6126 (comment)). Checking for existence of ReadableStream would be sufficient though (this was also done in the proposed pull request at #9050).

The user agent line is still necessary; Alternatively, feature detection can be used, as shown at:

var supportsMozChunked =
typeof PDFJSDev !== 'undefined' && PDFJSDev.test('CHROME') ? false :
(function supportsMozChunkedClosure() {
try {
var x = new XMLHttpRequest();
// Firefox 37- required .open() to be called before setting responseType.
// https://bugzilla.mozilla.org/show_bug.cgi?id=707484
// Even though the URL is not visited, .open() could fail if the URL is
// blocked, e.g. via the connect-src CSP directive or the NoScript addon.
// When this error occurs, this feature detection method will mistakenly
// report that moz-chunked-arraybuffer is not supported in Firefox 37-.
x.open('GET', globalScope.location.href);
x.responseType = 'moz-chunked-arraybuffer';
return x.responseType === 'moz-chunked-arraybuffer';
} catch (e) {
return false;
}
})();

@manmeet0307
Copy link

Hi. I would like to solve this issue. Please assign me this. And also, please guide me how do i move forward in contributing more to this repository !

s1iqbal pushed a commit to s1iqbal/pdf.js that referenced this issue Feb 6, 2018
s1iqbal pushed a commit to s1iqbal/pdf.js that referenced this issue Feb 6, 2018
@s1iqbal
Copy link

s1iqbal commented Feb 6, 2018

Resolved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
9 participants