*I've had this patch locally for awhile, but have apparently missed to upstream it.*
This simplifies enabling of new ESLint rules, since most of them support automatic fixing of errors, without having to edit `gulpfile.js` or manually invoke ESLint directly.
With the way that the `default_preferences.json` file is now generated at build time, the `gulp lint` task is now noticeably slower than before. This slowdown has been, and still is, somewhat annoying during the deployment of new ESLint rules.
Hence this patch, which moves the `chromium/preferences_schema.json` validation from `gulp lint` and into a new `gulp lint-chromium` task instead. *Obviously* this new task is run as part of the `gulp npm-test` task, and thus through `npm test` on Node.js/Travis, such that it's still being tested as before.
Having thought *briefly* about using `css-vars-ponyfill`, I'm no longer convinced that it'd be a good idea. The reason is that if we actually want to properly support CSS variables, then that functionality should be available in *all* of our CSS files.
Note in particular the `pdf_viewer.css` file that's built as part of the `COMPONENTS` target, in which case I really cannot see how a rewrite-at-the-client solution would ever be guaranteed to always work correctly and without accidentally touching other CSS in the surrounding application.
All-in-all, simply re-writing the CSS variables at build-time seems much easier and is thus the approach taken in this patch; courtesy of https://github.com/MadLittleMods/postcss-css-variables
By using its `preserve` option, the built files will thus include *both* a fallback and a modern `var(...)` format[1]. As a proof-of-concept this patch removes a couple of manually added fallback values, and converts an additional sidebar related property to use a CSS variable.
---
[1] Comparing the `master` branch with this patch, when using `gulp generic`, produces the following diff for the built `web/viewer.css` file:
```diff
@@ -408,6 +408,7 @@
:root {
--sidebar-width: 200px;
+ --sidebar-transition-duration: 200ms;
}
* {
@@ -550,27 +551,28 @@
position: absolute;
top: 32px;
bottom: 0;
- width: 200px; /* Here, and elsewhere below, keep the constant value for compatibility
- with older browsers that lack support for CSS variables. */
+ width: 200px;
width: var(--sidebar-width);
visibility: hidden;
z-index: 100;
border-top: 1px solid rgba(51, 51, 51, 1);
-webkit-transition-duration: 200ms;
transition-duration: 200ms;
+ -webkit-transition-duration: var(--sidebar-transition-duration);
+ transition-duration: var(--sidebar-transition-duration);
-webkit-transition-timing-function: ease;
transition-timing-function: ease;
}
html[dir='ltr'] #sidebarContainer {
-webkit-transition-property: left;
transition-property: left;
- left: -200px;
+ left: calc(-1 * 200px);
left: calc(-1 * var(--sidebar-width));
}
html[dir='rtl'] #sidebarContainer {
-webkit-transition-property: right;
transition-property: right;
- right: -200px;
+ right: calc(-1 * 200px);
right: calc(-1 * var(--sidebar-width));
}
@@ -640,6 +642,8 @@
#viewerContainer:not(.pdfPresentationMode) {
-webkit-transition-duration: 200ms;
transition-duration: 200ms;
+ -webkit-transition-duration: var(--sidebar-transition-duration);
+ transition-duration: var(--sidebar-transition-duration);
-webkit-transition-timing-function: ease;
transition-timing-function: ease;
}
```
After PR 9566, which removed all of the old Firefox extension code, the `FIREFOX` build flag is no longer used for anything.
It thus seems to me that it should be removed, for a couple of reasons:
- It's simply dead code now, which only serves to add confusion when looking at the `PDFJSDev` calls.
- It used to be that `MOZCENTRAL` and `FIREFOX` was *almost* always used together. However, ever since PR 9566 there's obviously been no effort put into keeping the `FIREFOX` build flags up to date.
- In the event that a new, Webextension based, Firefox addon is created in the future you'd still need to audit all `MOZCENTRAL` (and possibly `CHROME`) build flags to see what'd make sense for the addon.
Currently *all* users of `pdfjs-dist` are forced to install the `webpack` and `worker-loader` packages, despite the fact that they are *only* relevant if the `webpack.js` file is being used (with a custom Webpack build).
This really doesn't seem great, especially since those packages are the only remaining dependencies in the `pdfjs-dist` library, and it thus seem more reasonable overall that Webpack users handle those dependencies themselves.
To prevent unnecessarily cryptic runtime failures, when people update to newer `pdfjs-dist` versions, the `webpack.js` file was updated to explicitly check for the existence of the `worker-loader` package and error otherwise.
Furthermore, note that `webpack` was only listed as a dependency because of the `worker-loader` package itself (see issue 9248).
Obviously these changes may not be seen as great by Webpack users who rely on `pdfjs-dist`, since it forces them to handle the dependencies themselves, however it should improve things considerably for "general" users of `pdfjs-dist` by not burdening them with unnecessary dependencies.
These sort of changes are also in line with other recent changes, see PR 11418, which removed built-in fake worker loader code for specific JS builders/bundlers/frameworks. This work was prompted not only by a desire to simplify/clean-up old code, but also to lessen future support burden since the PDF.js contributors cannot be assumed to be experts in various JS bundlers.
This patch makes the follow changes:
- Remove no longer necessary inline `// eslint-disable-...` comments.
- Fix `// eslint-disable-...` comments that Prettier moved down, thus causing new linting errors.
- Concatenate strings which now fit on just one line.
- Fix comments that are now too long.
- Finally, and most importantly, adjust comments that Prettier moved down, since the new positions often is confusing or outright wrong.
Note that Prettier, purposely, has only limited [configuration options](https://prettier.io/docs/en/options.html). The configuration file is based on [the one in `mozilla central`](https://searchfox.org/mozilla-central/source/.prettierrc) with just a few additions (to avoid future breakage if the defaults ever changes).
Prettier is being used for a couple of reasons:
- To be consistent with `mozilla-central`, where Prettier is already in use across the tree.
- To ensure a *consistent* coding style everywhere, which is automatically enforced during linting (since Prettier is used as an ESLint plugin). This thus ends "all" formatting disussions once and for all, removing the need for review comments on most stylistic matters.
Many ESLint options are now redundant, and I've tried my best to remove all the now unnecessary options (but I may have missed some).
Note also that since Prettier considers the `printWidth` option as a guide, rather than a hard rule, this patch resorts to a small hack in the ESLint config to ensure that *comments* won't become too long.
*Please note:* This patch is generated automatically, by appending the `--fix` argument to the ESLint call used in the `gulp lint` task. It will thus require some additional clean-up, which will be done in a *separate* commit.
(On a more personal note, I'll readily admit that some of the changes Prettier makes are *extremely* ugly. However, in the name of consistency we'll probably have to live with that.)
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).
*Please note:* This patch does *not* prevent the 'default_preferences' task from running more than once during `npm test`, but it does ensure that the tasks won't run *concurrently* by running the relevant tests in *series*.
While it would obviously still make sense to re-factor the gulpfile to account for changes in `gulp` version 4, by at least tweaking the `npm test` command the intermittent failures on Travis should at least go away.
Unfortunately I forgot to test `gulp mozcentraldiff` with PR 11256, which is really bad since it will cause the en-US `chrome.properties` l10n file to be deleted at mozilla-central; sorry about breaking this!
In order to address this we'll have to re-add (only) the en-US `chrome.properties` l10n file, which is simple enough. Furthermore, since we're not doing any sort of build-specific parsing of the l10n files, we can just copy the en-US files as-is rather than having to run `gulp locale` during `gulp mozcentral`.
With the removal of the (standalone) Firefox building code in PR 9566 (a year and a half ago), these files are now completely unused in the GitHub repository[1].
Hence it doesn't really seem necessary to keep fetching them with `gulp importl10n`, and the existing files in the `l10n` folder can also be removed (thanks to version control, they're easy enough to restore should the need ever arise).
The patch also allows an additional simplification, for the `gulp locale` and `gulp mozcentral` commands, since it's now possible to stop writing `l10n` files to the `extensions/firefox/` folder and instead just copy them similar to other build targets.
---
[1] They're obviously still used in `mozilla-central`, for fallback messages displayed through `PdfStreamConverter.jsm`, but that doesn't make it necessary to keep them *here* as far as I'm concerned.
With the removal of the (standalone) Firefox building code in PR 9566 (a year and a half ago), these files are now completely unused.
Hence it doesn't really make sense to keep building them as part of `gulp locale`, and the existing files in the `l10n` folder can also be removed (thanks to version control, they're easy enough to restore should the need ever arise).
The following is printed with every build, which gets kind of annoying when looking at the logs:
```
Replace Autoprefixer browsers option to Browserslist config.
Use browserslist key in package.json or .browserslistrc file.
Using browsers option cause some error. Browserslist config
can be used for Babel, Autoprefixer, postcss-normalize and other tools.
If you really need to use option, rename it to overrideBrowserslist.
Learn more at:
https://github.com/browserslist/browserslist#readmehttps://twitter.com/browserslist
```
Given how we're using Autoprefixer, with a number of build-specific configs, simply changing the option name seems like the easiest solution here.
(I'm also adding a couple of newlines at the `autoprefixer` call-sites, to aid readability.)
Compared to the recently replaced `URL` polyfill, the new `ReadableStream` polyfill isn't being exported globally for two reasons:
- We're currently checking for the existence of a global `ReadableStream` implementation when determining if the Fetch API will be used; please see `isFetchSupported` in the src/display/display_utils.js file.
- Given that it's much newer functionality (compared to `URL`) and that not all browsers may implement all parts of the specification yet, not exposing the `ReadableStream` globally seems safer for now.
Note how at https://mozilla.github.io/pdf.js/api/ it's being described as API docs, however `src/core/annotation.js` is not part of the public API.
Furthermore, given that the code residing in the `src/core/` folder is run in a worker-thread, it's not even accessible on the main-thread (since `postMessage` is being used to transfer the data).
Hence the different API methods simply returns a "proxy" to the underlying data, but not actually the same objects and data structures as in the worker-thread itself; thus it doesn't make a whole lot of sense to expose this in API docs as far as I'm concerned.
Finally, the patch fixes a small JSDoc related typo in `src/display/api.js` when referring to the `TextStyle` typedef.
The generated `default_preferences.json` file is necessary when initializing the Firefox preferences, which only supports certain types, hence this patch adds additional validation to help prevent run-time errors in Firefox.
Given that these changes add a code-path to `AppOptions.getAll` which could throw, the `OptionKind.PREFERENCE` branch is also modified to require *exact* matching to prevent (future) errors in the viewer.
Finally the conditionally defined `defaultOptions` will no longer (potentially) be considered during the `gulp default_preferences` task, to make it more difficult for them to be accidentally included.
In order to, quickly, unblock future Webpack updates this patch implements a *temporary* work-around for the regression; please refer to issue 10177 for additional details.
Please note that this patch doesn't require *any* changes to the source code, but only (small) build-time changes to `gulpfile.js` and as such will be easy to revert.
Currently any editing of the preferences require updates in *three* separate files, which isn't a great developer experience to say the least.
This has annoyed me sufficiently to write this patch, which moves the definition of all preferences into `AppOptions` and adds a new `gulp` task to generate the `default_preferences.json` file for the builds where it's needed.
Based on the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1521413, this patch simply removes the `ReadableStream` polyfill completely from MOZCENTRAL builds.
With this patch, the size of the `gulp mozcentral` build target is thus further reduced (building on PR 10470):
| | `build/mozcentral`
|-------|-------------------
|master | 3 339 666
|patch | 3 209 572
With https://bugzilla.mozilla.org/show_bug.cgi?id=1505122 landing in Firefox 65, the native `ReadableStream` implementation is now enabled by default in Firefox.
Obviously it would be nice to simply stop bundling the polyfill in MOZCENTRAL builds altogether, however given that it's still possible to disable[1] `ReadableStream` this is probably not a good idea just yet.
Nonetheless, now that native support is available, it seems unnecessary (and wasteful) to keep bundling the polyfill twice[2] in MOZCENTRAL builds. Hence this patch, which contains a suggest approach for packing the polyfill in a *separate* file which is then *only* loaded if/when needed.
With this patch, the size of the `gulp mozcentral` build target is thus reduced accordingly:
| | `build/mozcentral`
|-------|-------------------
|master | 3 461 089
|patch | 3 340 268
Besides the PDF.js files taking up less space in Firefox this way, the additional benefit is that there's (by default) less code that needs to be loaded and parsed when the PDF Viewer is used which also cannot hurt.
---
[1] In `about:config`, by toggling the `javascript.options.streams` preference.
[2] Once in the `build/pdf.js` file, and once in the `build/pdf.worker.js` file.
This required the following changes in the Gulpfile:
- Defining a series of tasks is no longer done with arrays, but with the
`gulp.series` function. The `web` target is refactored to use a
smaller number of tasks to prevent tasks from running multiple times.
- Getting all tasks must now be done through the task registry.
- Tasks that don't return anything must call `done` upon completion.
Moreover, this upgrade allows us to use the latest Node.js on Travis CI
again.
This patch does three things:
- Updates the `gulp unittestcli` command, using `gulp lint` as a guide, such that it can be run locally on Windows without any modifications.
- Updates the `gulp lib` command to support disabling of Babel through the `SKIP_BABEL` environment variable. Note that all other build targets support this mode, and there's no good reason for `lib` to be any different here.
- Updates the `npm test` command, used in Node.js/Travis, to test the code as-is test. Since modern Node.js versions seem to have no problems with ES6 compatible code in general, we should just test the source code as-is instead (similar to the tests running on the regular bots).
Update configuration to work with Babel 7
Explicitly require globals - eslint-plugin-mozilla needs it, but doesn't require it on its own.
Fix Regexp to match Babel 7's inlined _interopRequireDefault
For proof-of-concept, this patch converts a couple of `Promise` returning methods to use `async` instead.
Please note that the `generic` build, based on this patch, has been successfully testing in IE11 (i.e. the viewer loads and nothing is obviously broken).
Being able to use modern JavaScript features like `async`/`await` is a huge plus, but there's one (obvious) side-effect: The size of the built files will increase slightly (unless `SKIP_BABEL == true`). That's unavoidable, but seems like a small price to pay in the grand scheme of things.
Finally, note that the `chromium` build target was changed to no longer skip Babel translation, since the Chrome extension still supports version `49` of the browser (where native `async` support isn't available).
This moves/exposes the `URL` polyfill similarily to the existing `ReadableStream` polyfill, rather than exposing it globally, to avoid interfering with any "outside" code.
Both the `URL` and `ReadableStream` polyfills are now exposed on the `pdfjsLib` object, such that they are accessible to the viewer components.
Furthermore, the `no-restricted-globals` ESLint rule is also enabled to prevent accidental usage of the native `URL`/`ReadableStream` implementations directly in the `src/` and `web/` folders; see also https://eslint.org/docs/rules/no-restricted-globals
Addresses the remaining TODO in https://github.com/mozilla/pdf.js/projects/6
Please note that the standalone `pdf.image_decoders.js` file will be including the complete `src/shared/util.js` file, despite only using parts of it.[1] This was done *purposely*, to not negatively impact the readability/maintainability of the core PDF.js code.
Furthermore, to ensure that the compatibility is the same in the regular PDF.js library *and* in the the standalone image decoders, `src/shared/compatibility.js` was included as well.
To (hopefully) prevent future complaints about the size of the built `pdf.image_decoders.js` file, a few existing async-related polyfills are being skipped (since all of the image decoders are completely synchronous).
Obviously this required adding a couple of pre-processor statements, but given that these are all limited to "compatibility" code, I think this might be OK!?
---
[1] However, please note that previous commits moved `PageViewport` and `MessageHandler` out of `src/shared/util.js` which reduced its size.
Since the tests (currently) run with the `pdf.worker.js` file built, i.e. with `PRODUCTION = true` set, there's no simple way to add e.g. `assert` calls for both non-production *and* test-only builds without also affecting PRODUCTION builds.
To avoid having to manually update the "shared files" list in `gulpfile.js`, whenever any changes are made to the file structure in the `src/shared/` folder, let's simply list the files we do *not* want instead.
Given that the excluded files have been consistent ever since the `lib` build target was added, this patch should help avoid unnecessary churn in `gulpfile.js` in the future.
