Adds UMD headers to core, display and shared files. #6683

yurydelendik · 2015-11-23T18:07:11Z

Now each file is a module. There are some benefits: we can track dependencies between files and various module loading systems can be used, e.g. requirejs or node. Not so good news that we have some circular dependencies to deal with (I fixed/patched couple of them), e.g. colorspace uses image and image uses colorspace. I tried not to fix or refactor anything in this PR (only stuff to make modules dependencies valid). That's just an intermediate step the clean up the dependencies.

jshint globals directives were removed where was possible. I kept only PDFJS, however in the future we can refactor that too. Notice that we may try to turn "unused" on now.

There is validation tool 'external/umdutils/verifier.js' that helps to maintain UMD headers valid. See for UMD format definition there.

Future work might include: use module loading system for the viewer, remove UMD module headers during bundling, remove src/expose_to_global.js (which was created for unit tests), split some files into smaller (e.g. core/fonts.js), refactor PDFJS global more. After this is done we can individually refactor each module and even migrate to some e.g. ES6 translator.

yurydelendik · 2015-11-23T18:12:07Z

After this patch you can execute the following in the node.js:

var api = require('./src/display/api.js');
PDFJS.displayWorker = true; // remove that in the future
var data = require('fs').readFileSync('./web/compressed.tracemonkey-pldi-09.pdf');
var loadingTask = api.getDocument(new Uint8Array(data));
loadingTask.promise.then(function (doc) {
  console.log(doc.numPages);
});

yurydelendik · 2015-11-23T19:06:11Z

Dependo generated graph (only core and display):

Rob--W · 2015-11-23T19:08:50Z

Questions:

The module names are repeated 5+ times:
- once in AMD
- once in CommonJS
- once without module loader
- once in the function expression's parameters
- at least once in the function body
Does your tool enforce that all of these names are consistent and follow the same convention (and briefly be documented somewhere)?
There is lots of boilerplate. Can the build tool be updated to automatically trim the duplicate information when a single-file build is generated? E.g. by only using AMD, CommonJS or the module pattern (whatever is the easiest for you).

yurydelendik · 2015-11-23T19:52:57Z

Does your tool enforce that all of these names are consistent and follow the same convention (and briefly be documented somewhere)?

The validation of the names is done by node make lint command (see also https://github.com/yurydelendik/pdf.js/blob/module-core/external/umdutils/verifier.js#L18). It also tracks circular dependencies. The AMD names (for module id and dependencies) looks like a good candidates to be selected as primary names (it easy to map them to CommonJS path and JS object name).

Can the build tool be updated to automatically trim the duplicate information when a single-file build is generated?

Yes. The intent will be eventually to build without UMD headers -- those can be replaced by regular function closure, e.g.

(function (root, (exports, sharedUtil, displayWebGL) {
  ... 
})((root.pdfjsDisplayPatternHelper = {}), root.pdfjsSharedUtil, root.pdfjsDisplayWebGL);

where root can be some local object instead of window.

Also, I think we shall remove all links to core/display/shared files and use module loaders instead (e.g. RequireJS).

(IHMO it's out of scope of this PR ATM -- I don't want to touch our build system without real need to)

yurydelendik · 2015-11-23T20:31:58Z

Let me also re-iterate how to choose names:

for AMD module name:
- almost all files at 'src/' folder shall start with 'pdfjs/' prefix followed by the relative path excluding the extension, e.g. 'src/core/pattern_helper.js' file will have 'pdfjs/core/pattern_helper' AMD module name
- The dependencies shall use full module names
- the name, as well as the file name, shall be only lower case ASCII letter and digits, and '_' can be used
for CommonJS reference:
- all references to the required modules must be relative the the current one, e.g. if 'src/core/pattern_helper.js' refers 'src/shared/util.js' the path shall be '../shared/utils.js'
- the path shall start from './' or '../' and ends with '.js'
for global/root name:
- the name is based on AMD name: after each '/' character, the letter is upper-cased, and the '/' and '_' characters removed (it's possible to uppercase other letter inside the name, e.g. pdfjsCoreColorSpace or pdfjsCorePatternHelper, but shall be consistent for a module name and its references)
import name:
- the name is part of the corresponding global/root name, but without 'pdfjs' prefix and first letter is in lower case

Snuffleupagus · 2015-11-24T11:27:37Z

src/core/document.js

@@ -542,3 +593,7 @@ var PDFDocument = (function PDFDocumentClosure() {

  return PDFDocument;
 })();
+
+exports.Paga = Page;


Nit: typo exports.Paga should be exports.Page.

Snuffleupagus · 2015-11-24T11:38:20Z

A testing related nit: shouldn't test/unit/obj_spec.js be renamed to test/unit/primitives_spec.js, since that unit-test covers code that is now moved from obj.js?

timvandermeij · 2015-11-24T21:51:33Z

Aside from the nit above, this PR needs a rebase too after the recently landed PRs.

timvandermeij · 2015-12-01T20:26:16Z

@yurydelendik Could you rebase this PR again? I think we should land this soon as it keeps on needing a rebase and it looks good to have modules and to drop most of the globals.

timvandermeij · 2015-12-01T21:25:19Z

/botio-linux preview

pdfjsbot · 2015-12-01T21:25:20Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/397b69541371361/output.txt

pdfjsbot · 2015-12-01T21:26:07Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/397b69541371361/output.txt

Total script time: 0.79 mins

Published

timvandermeij · 2015-12-01T21:27:51Z

/botio test

pdfjsbot · 2015-12-01T21:27:52Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/a8be5c2601c97ae/output.txt

pdfjsbot · 2015-12-01T21:27:52Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/1d5e50d933793ce/output.txt

pdfjsbot · 2015-12-01T21:46:55Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/a8be5c2601c97ae/output.txt

Total script time: 19.04 mins

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

pdfjsbot · 2015-12-01T21:48:11Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/1d5e50d933793ce/output.txt

Total script time: 20.31 mins

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

yurydelendik · 2015-12-07T02:18:00Z

New 'evaluator' on 'function' dependency is added by #6723 -- fixed.

Snuffleupagus · 2015-12-07T09:12:20Z

src/core/function.js

+exports.PDFFunction = PDFFunction;
+exports.PostScriptEvaluator = PostScriptEvaluator;
+exports.PostScriptCompiler = PostScriptCompiler;
+}));


Don't you also need to export isPDFFunction here, or am I missing something?

Yes, node make lint was throwing "ERROR:The non-exported symbol is referred: isPDFFunction from pdfjs/core/function used in pdfjs/core/evaluator". I think we need to change travis to execute that instead.

Snuffleupagus · 2015-12-11T15:06:48Z

external/umdutils/verifier.js

+    context.warnCallback(path + ': ' + msg);
+  }
+  function error(msg) {
+    context.info(path + ': ' + msg);


Shouldn't this call context.errorCallback instead?

Yes, thank you.

Snuffleupagus · 2015-12-11T15:49:10Z

external/umdutils/verifier.js

+  }
+  // Scans for exports definitions in the body.
+  var exportedNames = [];
+  re = /\bexports.(\w+)\s*=\s/g;


While testing the PR locally, I wanted to check that the validator failed if an exports.<name> wasn't present. In order to do that, I initially tested to just comment out a random exports.<name> line. To my surprise, that didn't fail, which lead me to this line.

So my question is: Should this regexp be changed to also check that the line with exports.<name> isn't commented out, since that would cause a runtime failure that the validator currently doesn't catch?

Ideally we shall catch all of those cases by testing. And we usually enforce that during code review by not allowing commented code to be present. This a temporary solution anyway to just validate introduction of UMDs and keeping code quality until ES6 module imports will be introduced.

That sounds fine to me!
(Since I happened to notice it, I figured that it couldn't hurt to ask.)

brendandahl · 2015-12-15T19:32:56Z

/botio test

pdfjsbot · 2015-12-15T19:32:57Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/876914199871804/output.txt

pdfjsbot · 2015-12-15T19:32:57Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/16c4337fe8456a5/output.txt

pdfjsbot · 2015-12-15T19:52:44Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/876914199871804/output.txt

Total script time: 19.77 mins

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

pdfjsbot · 2015-12-15T19:53:22Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/16c4337fe8456a5/output.txt

Total script time: 20.41 mins

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

Adds UMD headers to core, display and shared files.

yurydelendik force-pushed the module-core branch from 6f6787c to 513b9f9 Compare November 23, 2015 18:31

timvandermeij added the core label Nov 23, 2015

Snuffleupagus reviewed Nov 24, 2015
View reviewed changes

yurydelendik force-pushed the module-core branch from 513b9f9 to 529b5e1 Compare November 24, 2015 13:12

yurydelendik force-pushed the module-core branch 3 times, most recently from 663db97 to 1c71074 Compare December 1, 2015 18:20

yurydelendik force-pushed the module-core branch from 1c71074 to eafe35f Compare December 1, 2015 20:50

timvandermeij assigned brendandahl Dec 1, 2015

yurydelendik force-pushed the module-core branch 2 times, most recently from ca1dd1b to d804ac0 Compare December 5, 2015 13:10

yurydelendik mentioned this pull request Dec 6, 2015

Specify main #6729

Closed

yurydelendik force-pushed the module-core branch from d804ac0 to 2898e31 Compare December 7, 2015 02:16

Snuffleupagus reviewed Dec 7, 2015
View reviewed changes

yurydelendik force-pushed the module-core branch from 2898e31 to ebf1df0 Compare December 7, 2015 13:34

Snuffleupagus mentioned this pull request Dec 7, 2015

Let Travis lint using node make lint, instead of a hard-coded command #6733

Merged

timvandermeij mentioned this pull request Dec 9, 2015

Separate pdf parser into its own module #6351

Closed

yurydelendik force-pushed the module-core branch from ebf1df0 to bd9e17d Compare December 11, 2015 12:59

Snuffleupagus reviewed Dec 11, 2015
View reviewed changes

yurydelendik force-pushed the module-core branch from bd9e17d to e51285b Compare December 11, 2015 15:33

Snuffleupagus reviewed Dec 11, 2015
View reviewed changes

yurydelendik force-pushed the module-core branch from e51285b to de22058 Compare December 11, 2015 16:23

yurydelendik added 3 commits December 15, 2015 13:24

Adds UMD headers to core, display and shared files.

6b60c8f

UMD validation and generation tools.

450edc9

Allows requirejs and node load fake worker files.

b084dc0

yurydelendik force-pushed the module-core branch from de22058 to b084dc0 Compare December 15, 2015 19:24

brendandahl added a commit that referenced this pull request Dec 16, 2015

Merge pull request #6683 from yurydelendik/module-core

f1c64b6

Adds UMD headers to core, display and shared files.

brendandahl merged commit f1c64b6 into mozilla:master Dec 16, 2015

Snuffleupagus mentioned this pull request Dec 16, 2015

Only export Uint32ArrayView when it's actually defined, to prevent breaking e.g. the Firefox addon/built-in version #6762

Merged

This was referenced Dec 16, 2015

Collapses UMD headers in bundled files. #6764

Merged

Wrap pdf.js and pdf.worker.js with UMD module. #6775

Closed

yurydelendik deleted the module-core branch January 28, 2016 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds UMD headers to core, display and shared files. #6683

Adds UMD headers to core, display and shared files. #6683

yurydelendik commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

Rob--W commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

Snuffleupagus Nov 24, 2015

Snuffleupagus commented Nov 24, 2015

timvandermeij commented Nov 24, 2015

timvandermeij commented Dec 1, 2015

timvandermeij commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

timvandermeij commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

yurydelendik commented Dec 7, 2015

Snuffleupagus Dec 7, 2015

yurydelendik Dec 7, 2015

Snuffleupagus Dec 11, 2015

yurydelendik Dec 11, 2015

Snuffleupagus Dec 11, 2015

yurydelendik Dec 11, 2015

Snuffleupagus Dec 11, 2015

brendandahl commented Dec 15, 2015

pdfjsbot commented Dec 15, 2015

pdfjsbot commented Dec 15, 2015

pdfjsbot commented Dec 15, 2015

pdfjsbot commented Dec 15, 2015

Adds UMD headers to core, display and shared files. #6683

Adds UMD headers to core, display and shared files. #6683

Conversation

yurydelendik commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

Rob--W commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

yurydelendik commented Nov 23, 2015

Choose a reason for hiding this comment

Snuffleupagus commented Nov 24, 2015

timvandermeij commented Nov 24, 2015

timvandermeij commented Dec 1, 2015

timvandermeij commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 1, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 1, 2015

pdfjsbot commented Dec 1, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 1, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 1, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 1, 2015

From: Bot.io (Linux)

Success

yurydelendik commented Dec 7, 2015

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

brendandahl commented Dec 15, 2015

pdfjsbot commented Dec 15, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 15, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 15, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 15, 2015

From: Bot.io (Linux)

Success