[api-minor] Produce non-translated/non-polyfilled builds by default

[Snuffleupagus]

No description provided.

[timvandermeij]

Here, and similar below, we could extract the function and give it skipBabel parameter so we don't have to duplicate it (unless I'm overlooking something, they seem equal aside from the SKIP_BABEL parameter). Something like (untested):

function createGenericViewer(skipBabel) {
   ...
   return merge([...]);
}

gulp.task('generic', gulp.series('buildnumber', 'default_preferences', 'locale', function() {
  createGenericViewer(/* skipBabel */ true);
}));

gulp.task('generic-es5', gulp.series('buildnumber', 'default_preferences', 'locale', function() {
  createGenericViewer(/* skipBabel */ false);
}));

[Snuffleupagus]

[...] they seem equal aside from the SKIP_BABEL parameter

For what it's worth, there's also a difference in output directories...

While you're obviously correct that it should be possible to reduce the size of the gulpfile.js changes, I'd very much like to get an OK on the general idea of this PR before spending too much time on it.

In no particular order, there's a couple of things that we'd need to consider:

• Are we OK with changing the default to producing non-translated/non-polyfilled builds by default, and instead having to explicitly specify when ES5-compatible builds are wanted?
  Personally I'm hoping this will be seen as a step in the right direction, since it might also allow further modernizing of the build system.
• ES5-compatible generic builds are obviously still provided, but is [api-minor] enough since these changes may require that implementers manually change what files they depend on?
  Also, are we OK with only providing pre-built ES5-compatible files for the generic build target, since this simplifies things considerably?
• What about all of the examples?
  Personally I'd like to avoid changing them wholesale to use the ES5-compatible files, but how should we document this (inline comments, Wiki entry, and/or something else; and will people even bother to read it)?

[timvandermeij]

I'm personally also in favor of preferring ES6 builds over ES5 builds. However, I'm not sure if [api-minor] is the right classification for this change since it's quite a behavior change; it feels more like [api-major] to me.

I'm thinking that to make this work for [api-minor] we can do the reverse, namely keep the default at ES5 but also provide ES6 builds. This is already a step in the right direction because now we don't really provide ES6 builds, so it will allow people to start using it and provide us with feedback. Moreover, we and users don't need to adjust anything immediately. For the next major version, we can then swap the builds, making ES6 the default and ES5 the alternative option. In the major version after that, we can then hopefully drop the ES5-only build altogether.

What do you think? Also /cc @yurydelendik @brendandahl for opinions on this.

[Snuffleupagus]

I'm thinking that to make this work for [api-minor] we can do the reverse, namely keep the default at ES5 but also provide ES6 builds.

Honestly, this feels like way more work/complication compared to the current approach (and certainly not something I'd bother doing, unless the users complaining about there not being any non-translated builds actually compensating me for my time :-).

Even trying to do something like this would also make e.g. the pdfjs-dist repository really big and complicated, not to mention even slower to generate, since you'd probably need to essentially duplicate all of it's code which is precisely what I actively tried to avoid here.
In that case I'd personally much rather just immediately increment the major version, and keep the current approach of modern builds with a few exceptions, and then be done with it since this simplifies things considerably!

Furthermore, note that the non-translated builds aren't actually ECMAScript 2015 (i.e. older classification ES6) but rather at least ECMAScript 2018 (i.e. older classification ES9). Hence labelling something as ES6 isn't even remotely accurate/correct, unfortunately.

[Snuffleupagus]

Sorry if the above seemed a bit grumpy!

Some additional thoughts:

[...] so it will allow people to start using it and provide us with feedback.

Assuming that a modern, and up-to-date, standards-compliant browser is being used I really cannot imagine there being any problems with using a non-translated/non-polyfilled build (based on the fact that the PDF Viewer in Firefox has been shipping this way for years).

Also, simply incrementing the major version should definitely allow to move forward and "fix" things like e.g. issue #11211.
Finally, considering that the first official release from the 2.0 branch was released a year ago, it doesn't seem so bad to me to increase that now (especially for these kind of build improvements).

[timvandermeij]

Incrementing the major version number for the reasons you mentioned sounds like a good idea, so perhaps that's indeed an easier approach. I'll think about this plan a bit more.

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/ee02a2e381b03ca/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.67.70.0:8877/ee02a2e381b03ca/output.txt

Total script time: 2.39 mins

Published

• Viewer: http://54.67.70.0:8877/ee02a2e381b03ca/web/viewer.html

[Snuffleupagus]

Part of this work probably ought to also include some sort of clean-up/re-factoring/simplification of the fake worker loading code, since the current state feels like a mess (and given the complaints about it):

pdf.js/src/display/api.js

Lines 49 to 106 in bbd2386

	let fakeWorkerFilesLoader = null;
	if (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('GENERIC')) {
	let useRequireEnsure = false;
	// For GENERIC build we need to add support for different fake file loaders
	// for different frameworks.
	if (typeof window === 'undefined') {
	// node.js - disable worker and set require.ensure.
	isWorkerDisabled = true;
	if (typeof __non_webpack_require__.ensure === 'undefined') {
	__non_webpack_require__.ensure = __non_webpack_require__('node-ensure');
	}
	useRequireEnsure = true;
	} else if (typeof __non_webpack_require__ !== 'undefined' &&
	typeof __non_webpack_require__.ensure === 'function') {
	useRequireEnsure = true;
	}
	if (typeof requirejs !== 'undefined' && requirejs.toUrl) {
	fallbackWorkerSrc = requirejs.toUrl('pdfjs-dist/build/pdf.worker.js');
	}
	const dynamicLoaderSupported =
	typeof requirejs !== 'undefined' && requirejs.load;
	fakeWorkerFilesLoader = useRequireEnsure ? (function() {
	return new Promise(function(resolve, reject) {
	__non_webpack_require__.ensure([], function() {
	try {
	let worker;
	if (typeof PDFJSDev !== 'undefined' && PDFJSDev.test('LIB')) {
	worker = __non_webpack_require__('../pdf.worker.js');
	} else {
	worker = __non_webpack_require__('./pdf.worker.js');
	}
	resolve(worker.WorkerMessageHandler);
	} catch (ex) {
	reject(ex);
	}
	}, reject, 'pdfjsWorker');
	});
	}) : dynamicLoaderSupported ? (function() {
	return new Promise(function(resolve, reject) {
	requirejs(['pdfjs-dist/build/pdf.worker'], function(worker) {
	try {
	resolve(worker.WorkerMessageHandler);
	} catch (ex) {
	reject(ex);
	}
	}, reject);
	});
	}) : null;
	if (!fallbackWorkerSrc && typeof document === 'object' &&
	'currentScript' in document) {
	const pdfjsFilePath = document.currentScript && document.currentScript.src;
	if (pdfjsFilePath) {
	fallbackWorkerSrc =
	pdfjsFilePath.replace(/(\.(?:min\.)?js)(\?.*)?$/i, '.worker$1$2');
	}
	}
	}

[Snuffleupagus]

[...] clean-up/re-factoring/simplification of the fake worker loading code,

I've tried to strip this down to its bare essentials, see master...Snuffleupagus:fakeWorkerLoader, with the intention of only supporting Node.js and the LIB build target out-of-the-box.
However, doing so causes some issues with the webpack example; while it still runs successfully there's some Critical dependency: the request of a dependency is an expression errors during the build step. (You'd expect that most users will rely on pre-built files from e.g. pdfjs-dist, so I'm not sure how much this would actually matter...)

All in all, I'm nowhere near knowledgeable enough about various module loaders to be able to sort out issues like these unfortunately. Hence I'm not really sure about the way forward here (and given that these sort of module loader changes brings no benefit to me personally, I'm finding it really hard to justify spending even more of my spare time on it).

[Snuffleupagus]

[...] with the intention of only supporting Node.js and the LIB build target out-of-the-box.

Although I suppose that you could just remove all non-browser "fakeWorkerLoader" fallback code and simply update the Node.js examples and gulp lib output to manually load the pdf.worker.js file, but I'm not sure if that would be considered an overall bad move!?

[timvandermeij]

I think the problem is that we're all not really knowledgeable about module loaders and we also don't have contributors maintaining this. Looking at #9580 perhaps it's not even desired to have support for Webpack in this repository? I think we should go back to basics a bit: support only the use cases that are relevant and most often used, and leave the rest to third-party implementations or contributors if they really wish to have this support in the repository.

@yurydelendik @brendandahl What do you think?

[Snuffleupagus]

However, doing so causes some issues with the webpack example; while it still runs successfully there's some Critical dependency: the request of a dependency is an expression errors during the build step.

This is, as far some people are concerned (me included), a bug in Webpack since it's currently impossible to force Webpack to leave a require statement alone in a general way; please see webpack/webpack#8826.

The only "solution" that currently seem to exist, as gross as it would be, is probably to use eval('require') since Webpack cannot parse that...
Given that the code-path in question is, or at least should be, protected by a runtime isNode check using eval is perhaps not actually as bad as it first seems!?

[timvandermeij]

Ugh. If it's required to be able to move on, I guess it's better than doing nothing, but it's also not great either. Don't we have the __non_webpack_require__ thing to tell Webpack to not touch the require statement though? I think we use it in a few places just for this purposes as far as I'm aware, although I must admit not to know much about this, but maybe it helps: https://github.com/mozilla/pdf.js/search?q=__non_webpack_require__&unscoped_q=__non_webpack_require__

[Snuffleupagus]

Don't we have the __non_webpack_require__ thing to tell Webpack to not touch the require statement though?

Yes, but in the built files that's replaced with a standard require (as expected) and that causes problems e.g. with https://github.com/mozilla/pdf.js/tree/master/examples/webpack (which uses the pdfjs-dist files) in the form of Critical dependency: ... warnings and also with third-party bundling such as e.g. issue #10253.

[timvandermeij]

Having thought a bit more about this I'm also not opposed to making this an [api-minor] change anymore. I do wonder if it's possible to tweak the download page a bit by not showing the buttons in the ES5-compatible section for older releases since those links are now broken on the page you linked.

[Snuffleupagus]

I do wonder if it's possible to tweak the download page a bit by not showing the buttons in the ES5-compatible section for older releases since those links are now broken on the page you linked.

Excellent point; yes that'll be somewhat of a problem, until the releases "catch" up to the new format.

I'm not sure how to fix this generally, short of manually hard-coding the links after the relevant releases are done (perhaps that's not so bad though, since it'd be temporary)?

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/a36bdfddf296cae/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.67.70.0:8877/a36bdfddf296cae/output.txt

Total script time: 2.41 mins

Published

• Viewer: http://54.67.70.0:8877/a36bdfddf296cae/web/viewer.html

[Snuffleupagus]

I'm not sure how to fix this generally, short of manually hard-coding the links after the relevant releases are done (perhaps that's not so bad though, since it'd be temporary)?

The latest version of the patch does something along those line, and will thus require manual fix-ups after the next two releases but then "things should just work"^TM.

[timvandermeij]

I'm fine with the approach taken here. If this is merged, I'll extend the release wiki page so we don't forget to manually update the website.

[Snuffleupagus]

I'm fine with the approach taken here. If this is merged, I'll extend the release wiki page so we don't forget to manually update the website.

Thanks for the review, hopefully I've managed to address all your comments.

/botio-linux preview

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/4d6a7c9d4dd6b7a/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.67.70.0:8877/4d6a7c9d4dd6b7a/output.txt

Total script time: 2.52 mins

Published

• Viewer: http://54.67.70.0:8877/4d6a7c9d4dd6b7a/web/viewer.html

[Snuffleupagus]

/botio test

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/249570a207f0a54/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/19f8f0b8546dc33/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.215.176.217:8877/19f8f0b8546dc33/output.txt

Total script time: 2.17 mins

• Font tests: FAILED
• Unit tests: FAILED
• Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/19f8f0b8546dc33/reftest-analyzer.html#web=eq.log

[pdfjsbot]

From: Bot.io (Linux m4)

---

Failed

Full output at http://54.67.70.0:8877/249570a207f0a54/output.txt

Total script time: 19.74 mins

• Font tests: Passed
• Unit tests: Passed
• Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/249570a207f0a54/reftest-analyzer.html#web=eq.log

[timvandermeij]

Let's do this. I'm seeing this as a good step forward. Thanks!

[Snuffleupagus]

@timvandermeij Thanks for good review comments!

I've opened a follow-up PR to clean-up the state on the bots a bit, see mozilla/botio-files-pdfjs#26