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 the missing PDF unit test for the API in NodeJS #9791

Closed
timvandermeij opened this issue Jun 9, 2018 · 3 comments
Closed

Enable the missing PDF unit test for the API in NodeJS #9791

timvandermeij opened this issue Jun 9, 2018 · 3 comments
Labels

Comments

@timvandermeij
Copy link
Contributor

The objective is to remove https://github.com/mozilla/pdf.js/blob/master/test/unit/api_spec.js#L166-L170 by fixing up src/display/node_stream.js to throw MissingPDFException when a PDF file is not found. Right now, this file is not handling errors in a consistent or even correct way compared with src/display/network.js and src/display/fetch_stream.js. Using the createResponseStatusError utility function might not make much sense in a Node.js environment. However, at the very least it seems that MissingPDFException or UnknownErrorException when we cannot tell that a PDF file is missing should be thrown manually. As is, the API (i.e., getDocument) is not returning the expected errors when loading fails in Node.js environments (as evident from this pending API unit test).

@AbhimanyuVashisht
Copy link
Contributor

I am working on it

@RonLek
Copy link
Contributor

RonLek commented Jul 18, 2018

@timvandermeij Do you think using https.get(sourceUrl, callback) as used here:
https://nodejs.org/api/https.html#https_https_get_options_callback over here

function parseUrl(sourceUrl) {

and throwing a MissingPDFException if res.statusCode===404 || res.statusCode === 0 && /^file:/.test(sourceUrl) would help?

@timvandermeij
Copy link
Contributor Author

I think the problem is that you're requesting the file twice then: once for this check and once to actually load the file. It's probably better to handle this in the spot where we're actually attempting to load the file. Perhaps it would help to enable the test locally, see where it breaks and backtrace from there.

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

No branches or pull requests

3 participants