[api-minor] Simplify the *fallback* fake worker loader code in src/display/api.js
#11418
Conversation
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/0b8abd3983448ab/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/a05f5728fa198f5/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/a05f5728fa198f5/output.txt Total script time: 18.80 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/0b8abd3983448ab/output.txt Total script time: 26.56 mins
|
Snuffleupagus
force-pushed
the
simplify-fakeWorkerLoader
branch
from
53c4e08
to
3c33cc7
Compare
…isplay/api.js` For performance reasons, and to avoid hanging the browser UI, the PDF.js library should *always* be used with web workers enabled. At this point in time all of the supported browsers should have proper worker support, and Node.js is thus the only environment where workers aren't supported. Hence it no longer seems relevant/necessary to provide, by default, fake worker loaders for various JS builders/bundlers/frameworks in the PDF.js code itself.[1] In order to simplify things, the fake worker loader code is thus simplified to now *only* support Node.js usage respectively "normal" browser usage out-of-the-box.[2] *Please note:* The officially intended way of using the PDF.js library is with workers enabled, which can be done by setting `GlobalWorkerOptions.workerSrc`, `GlobalWorkerOptions.workerPort`, or manually providing a `PDFWorker` instance when calling `getDocument`. --- [1] Note that it's still possible to *manually* disable workers, simply my manually loading the built `pdf.worker.js` file into the (current) global scope, however this's mostly intended for testing/debugging purposes. [2] Unfortunately some bundlers such as Webpack, when used with third-party deployments of the PDF.js library, will start to print `Critical dependency: ...` warnings when run against the built `pdf.js` file from this patch. The reason is that despite the `require` calls being protected by *runtime* `isNodeJS` checks, it's not possible to simply tell Webpack to just ignore the `require`; please see [Webpack issue 8826](https://github.com/webpack/webpack) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack).
Given that this code isn't needed "globally" in the file, it seems reasonable to move it to where it's actually used instead.
….workerSrc` in Node.js There's no particularily good reason, as far as I can tell, to not support a custom worker path in Node.js environments (even if workers aren't supported). This patch thus make the Node.js fake worker loader code-path consistent with the fallback code-path used with *browser* fake worker loader. Finally, this patch also deprecates[1] the `fallbackWorkerSrc` functionality, except in Node.js, since the user should *always* provide correct worker options since the fallback is nothing more than a best-effort solution. --- [1] Although it probably shouldn't be removed until the next major version.
…s`), and the `loadFakeWorker` function (in `web/app.js`) This patch reduces some duplication, by moving *all* fake worker loader code into the `setupFakeWorkerGlobal` function. Furthermore, the functions are simplified further by using `async`/`await` where appropriate.
… dependency: ...` warnings from Webpack Since bundlers, such as Webpack, cannot be told to leave `require` statements alone we are thus forced to jump through hoops in order to prevent these warnings in third-party deployments of the PDF.js library; please see [Webpack issue 8826](https://github.com/webpack/webpack) and libraries such as [require-fool-webpack](https://github.com/sindresorhus/require-fool-webpack). *Please note:* This is based on the assumption that code running in Node.js won't ever be affected by e.g. Content Security Policies that prevent use of `eval`. If that ever occurs, we should revert to a normal `require` statement and simply document the Webpack warnings instead.
Snuffleupagus
force-pushed
the
simplify-fakeWorkerLoader
branch
from
3c33cc7
to
d370037
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/10f8458c17fc148/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/5f344631f912bd9/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/10f8458c17fc148/output.txt Total script time: 18.97 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/5f344631f912bd9/output.txt Total script time: 26.42 mins
Image differences available at: http://54.215.176.217:8877/5f344631f912bd9/reftest-analyzer.html#web=eq.log |
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/60ba415ed5d3778/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/60ba415ed5d3778/output.txt Total script time: 1.75 mins Published |
|
Looks much simpler this way. I always found this part of the code base to be hard to read, so thank you for simplifying this! |
Completely agree regarding the "hard to read" part; and hopefully people aren't using fake workers anyway :-) |
|
@timvandermeij Also, I forgot to say thank you for landing this :-D Given that there's been a couple of |
|
Yes, I'll plan a release for next week. It has been a while since the last release, so it sounds like a good plan. |
Please refer to the individual commit messages for additional details.