Change to conditions that pdfjsLib
#8154
Conversation
There was a problem hiding this comment.
As asked for in #8146 (comment), did you test this patch with https://github.com/mozilla/pdf.js/tree/master/examples/webpack to ensure that the example still works?
web/pdfjs.js
Outdated
| if (typeof window !== 'undefined' && window['pdfjs-dist/build/pdf']) { | ||
| pdfjsLib = window['pdfjs-dist/build/pdf']; | ||
| } | ||
| else if (typeof require === 'function') { |
There was a problem hiding this comment.
As you can see in https://travis-ci.org/mozilla/pdf.js/builds/210076791#L501, this causes ESLint failures. Please note that the coding style calls for } and else to be placed on the same line, i.e. } else if (...) { (and similar below).
web/pdfjs.js
Outdated
| pdfjsLib = require('../build/pdf.js'); | ||
| } | ||
| else { | ||
| throw 'Neither `require` nor `window` found'; |
There was a problem hiding this comment.
Nit: Please use throw new Error(...) instead.
|
@Snuffleupagus, yes. just posted to the issue the following info: i've run the unit tests for the project and see that: ... next, i built the webpack bundles: ... then fired up the example webpack as you suggested. in the browser, here's what i see... |
web/pdfjs.js
Outdated
| } else { | ||
| pdfjsLib = window['pdfjs-dist/build/pdf']; // loaded via html script tag | ||
| throw 'Neither `require` nor `window` found'; |
There was a problem hiding this comment.
As previously mentioned, please change this to throw new Error(...); instead.
Also, once you've made the changes, please squash the commits into one such that this is ready for review (see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for more on this matter).
There was a problem hiding this comment.
woops. sure thing ;)
…se where webpack is not used. Updating brace style. Updating to throw new error vs. throwing a string.
mysterlune
force-pushed
the
pdfjslib_req_vs_window
branch
from
f226972 to
3aeef84
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/ff4514644457073/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/ff4514644457073/output.txt Total script time: 2.14 mins Published |
|
Looks good. Thank you for the patch. |
Change to conditions that `pdfjsLib`
Changing the order of logical conditions to prefer in the case where webpack is not used.
In the case where webpack is not being used (presumably in a server-side require context) for the distribution, the case which uses
requirepresumes that the require implementation can use a relative path to find the module.However, the
../build/pdf.jsfile may not be available torequireas expected. The value set onwindowis certainly available in this case. So my thinking here is to prefer thewindowvalue for settingpdfjsLibwhere it is available and fall back torequireif doing the rendering on the server-side (whererequirewill be more free to find thepdf.jsmodule by relative path).