Replaces RequireJS to SystemJS. #8050

yurydelendik · 2017-02-09T15:58:25Z

Removes RequireJS dependency since it will be hard to adapt ES6 module transpilers. It will be easier with SystemJS. Questions to resolve:

Central configuration (vs SystemJS.config in multiple places)
Dependency on Promise for legacy browsers (increase bar or move Promise polyfill to compatibility.js), e.g. we use that in some examples.

Snuffleupagus · 2017-02-09T17:50:02Z

Dependency on Promise for legacy browsers (increase bar or move Promise polyfill to compatibility.js), e.g. we use that in some examples.

Personally I feel that support for older browsers are now seriously holding us back from using many of the really nice new features in ES6, and I would wholeheartedly support increasing the minimum requirements for using PDF.js.

My suggestion is that version 1.7.225 (the current pre-release) should be the last version with support for pre-ES6 browsers, and that going forward only ES6 compatible browsers/environments will be supported.

Such a requirement would mean that most, if not all, compatibility code could be removed; furthermore it'd also reduce the support burden since we'd no longer need to add any new compatibility hacks (which would be nice, given the limited number of regular PDF.js contributors).
Obviously such a decision would need to be communicated publicly not just in the Wiki/README, but also on the mailing list/Twitter first. However I really think that this is the way forward here!

brendandahl · 2017-02-10T18:26:23Z

Personally I feel that support for older browsers are now seriously holding us back from using many of the really nice new features in ES6, and I would wholeheartedly support increasing the minimum requirements for using PDF.js.

We can use a transpiler (like babel) to support old browsers and still have all the shiny new stuff.

yurydelendik · 2017-02-25T19:34:16Z

/botio test

timvandermeij · 2017-02-25T19:53:59Z

examples/svgviewer/viewer.js

-require(['pdfjs/display/api', 'pdfjs/display/svg', 'pdfjs/display/global'],
-    function (api, svg, global) {
+// In production, the bundled pdf.js shall be used instead of SystemJS.
+SystemJS.config({


Do we still need this now that we have systemjs.config.js that seems to define this correctly? Compare to examples/helloworld/hello.js that also doesn't have this.

timvandermeij · 2017-02-25T19:57:25Z

src/worker_loader.js

@@ -15,21 +15,20 @@

 'use strict';

-if (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('PRODUCTION')) {


Why is this change necessary? Shouldn't this block be moved to compatibility.js?

It's needed only for requirejs (and probably for SystemJS as well) -- our tests were failing without it. Since we are not using src/worker_loader.js in the production anyway, I decided to remove the pre-processor from here.

timvandermeij · 2017-02-25T19:59:34Z

systemjs.config.js

@@ -0,0 +1,45 @@
+/* Copyright 2012 Mozilla Foundation


Let's change this to 2017.

timvandermeij · 2017-02-25T20:00:17Z

systemjs.config.js

+  if (typeof document !== 'undefined') {
+    baseLocation = new URL('./', document.currentScript.src);
+  } else if (typeof location !== 'undefined') {
+     // Probably worker -- walking subfolders until we will reach root.


Nit: one space too much here in the indentation

timvandermeij · 2017-02-25T20:04:12Z

test/test_slave.html

+                 SystemJS.import('pdfjs/display/global'),
+                 SystemJS.import('pdfjs/shared/util')])
+          .then(function (modules) {
+      var pdfjsSharedUtil = modules[4];


I think we can get rid of this intermediate variable and make the next line window.pdfjsSharedUtil = modules[4]; directly. The intermediate variable doesn't provide more clarity about what's happening.

timvandermeij · 2017-02-25T20:05:24Z

test/unit/jasmine-boot.js

-                 'pdfjs-test': '..'}});
-  require(['pdfjs/display/global', 'pdfjs-test/unit/annotation_spec',
+    Promise.all([
+           'pdfjs/display/global', 'pdfjs-test/unit/annotation_spec',


The indentation is inconsistent with the rest of the files. I'd prefer if we keep the same indentation as we do for other files here as well, i.e., indent with two spaces (I don't mind the diff being a few lines larger).

timvandermeij · 2017-02-25T20:07:44Z

web/viewer.js

+    Promise.all([SystemJS.import('pdfjs-web/app'),
+                 SystemJS.import('pdfjs-web/pdf_print_service')])
+           .then(function (modules) {
+      var web = modules[0];


Can we call this variable app instead? It's a bit confusing because web for me refers to the entire web folder, whereas app is clearly only app.js.

timvandermeij · 2017-02-27T21:56:20Z

/botio-linux preview

pdfjsbot · 2017-02-27T21:56:21Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/d1d29810130162e/output.txt

pdfjsbot · 2017-02-27T21:58:31Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/d1d29810130162e/output.txt

Total script time: 2.16 mins

Published

timvandermeij · 2017-02-27T22:00:10Z

/botio test

pdfjsbot · 2017-02-27T22:00:12Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/199f213dbd9f162/output.txt

pdfjsbot · 2017-02-27T22:00:13Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 1

Live output at: http://54.215.176.217:8877/296868ca9d08d8e/output.txt

pdfjsbot · 2017-02-27T22:22:28Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/296868ca9d08d8e/output.txt

Total script time: 22.10 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-02-27T22:30:04Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/199f213dbd9f162/output.txt

Total script time: 29.85 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2017-02-27T22:32:00Z

Thank you for working on this!

Replaces RequireJS to SystemJS.

yurydelendik added this to In Progress in ES6 modules Feb 9, 2017

yurydelendik added the 4-work-in-progress label Feb 9, 2017

yurydelendik changed the title ~~[WIP] Replaces RequireJS to SystemJS.~~ Replaces RequireJS to SystemJS. Feb 9, 2017

timvandermeij added the other label Feb 9, 2017

yurydelendik mentioned this pull request Feb 24, 2017

Move compatibility code to the shared/compatibility.js. #8102

Merged

yurydelendik force-pushed the systemjs branch from fcf059b to 9c9d903 Compare February 25, 2017 19:33

yurydelendik removed the 4-work-in-progress label Feb 25, 2017

timvandermeij reviewed Feb 25, 2017

View reviewed changes

Replaces RequireJS to SystemJS.

5b50e0d

yurydelendik force-pushed the systemjs branch from 9c9d903 to 5b50e0d Compare February 27, 2017 14:34

timvandermeij approved these changes Feb 27, 2017

View reviewed changes

timvandermeij merged commit 25f772a into mozilla:master Feb 27, 2017

timvandermeij moved this from In progress to Done in ES6 modules Feb 27, 2017

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8050 from yurydelendik/systemjs

5d0ed06

Replaces RequireJS to SystemJS.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces RequireJS to SystemJS. #8050

Replaces RequireJS to SystemJS. #8050

yurydelendik commented Feb 9, 2017 •

edited

Snuffleupagus commented Feb 9, 2017 •

edited

brendandahl commented Feb 10, 2017

yurydelendik commented Feb 25, 2017

timvandermeij Feb 25, 2017

timvandermeij Feb 25, 2017

yurydelendik Feb 27, 2017

timvandermeij Feb 25, 2017

timvandermeij Feb 25, 2017

timvandermeij Feb 25, 2017

timvandermeij Feb 25, 2017

timvandermeij Feb 25, 2017

timvandermeij commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

timvandermeij commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

timvandermeij commented Feb 27, 2017

		@@ -15,21 +15,20 @@

		'use strict';

		if (typeof PDFJSDev === 'undefined' \|\| !PDFJSDev.test('PRODUCTION')) {

Replaces RequireJS to SystemJS. #8050

Replaces RequireJS to SystemJS. #8050

Conversation

yurydelendik commented Feb 9, 2017 • edited

Snuffleupagus commented Feb 9, 2017 • edited

brendandahl commented Feb 10, 2017

yurydelendik commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 27, 2017

From: Bot.io (Linux)

Success

Published

timvandermeij commented Feb 27, 2017

pdfjsbot commented Feb 27, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 27, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 27, 2017

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 27, 2017

From: Bot.io (Linux)

Success

timvandermeij commented Feb 27, 2017

yurydelendik commented Feb 9, 2017 •

edited

Snuffleupagus commented Feb 9, 2017 •

edited