Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds/modifies examples for node.js and webpack. #6785

Merged
merged 1 commit into from
Dec 21, 2015

Conversation

yurydelendik
Copy link
Contributor

Modifies and cleans up node.js examples to use pdfjs-dist bundle instead of pdf.combined.js

Introduces webpack example to demonstrate proper way to disconnect worker part from display modules. See examples/webpack/README.md for details.

warn('Setting up fake worker.');
globalScope.PDFJS.disableWorker = true;
if (!globalScope.PDFJS.disableWorker) {
warn('Setting up fake worker.');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this warning appear only when PDF.js decides to fall back (and not when user explicitly sets the option).

global.DOMParser = require('./domparsermock.js').DOMParserMock;

require('../../build/singlefile/build/pdf.combined.js');
// Run `node make dist` to generate 'pdfjs-dist' npm package.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's keep this the same as in the other example file and add "files" after "package", i.e., make it "Run node make dist to generate 'pdfjs-dist' npm package files."

@timvandermeij
Copy link
Contributor

We need to adjust https://github.com/mozilla/pdf.js/wiki/Setup-PDF.js-in-a-website after this lands. We can probably remove the information about webpack and just point to the examples.

@yurydelendik yurydelendik mentioned this pull request Dec 21, 2015
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/36e378baee529f4/output.txt

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/1e10f8a981e3843/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/1e10f8a981e3843/output.txt

Total script time: 20.29 mins

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

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

Total script time: 20.64 mins

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

timvandermeij added a commit that referenced this pull request Dec 21, 2015
Adds/modifies examples for node.js and webpack.
@timvandermeij timvandermeij merged commit 05b9d37 into mozilla:master Dec 21, 2015
@timvandermeij
Copy link
Contributor

Looks good, thank you!

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

This caused a regression. node-ensure is loaded from root:

Request URL:http://localhost/node-ensure.js
Request Method:GET
Status Code:404 Not Found

@@ -5,6 +5,7 @@
"jsdoc": "^3.3.0-alpha9",
"jshint": "~2.8.0",
"wintersmith": "^2.0.0",
"node-ensure": "^0.0.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it and why it's needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's needed for webpack to properly split bundles

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the problem is that this is in devDependencies, not runtime dependencies?

@yurydelendik
Copy link
Contributor Author

This caused a regression.

@Vanuan Please be more specific. Can you provide examples (preferably online)?

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

Previously, pdf-dist worked fine in browser. Now it tries to load non-existing file "node-ensure.js".

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

node-ensure documentation reads:

NOTE: This module is not compatible with Browserify.

@yurydelendik
Copy link
Contributor Author

@Vanuan there is no support of SystemJS or Browserify for PDF.js yet. (It's coincidence that it worked before) Please file issues and provide short examples -- that will speed up resolution of the issues.

https://manual-form-digitizing-vanuan.c9users.io/

Looks like at https://manual-form-digitizing-vanuan.c9users.io/ systemjs was used. For some reason it provides (?) module and module.require object and that confuses our frameworks.js code.

This module is not compatible with Browserify.

You can skip 'node-ensure' in the Browserify during build step. (We don't have example or docs for that yet)

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

Ok, I'll probably submit my own npm module

@yurydelendik
Copy link
Contributor Author

Ok, I'll probably submit my own npm module

It's your choice, but we would prefer that somebody else would add support of those frameworks to the core. Notice that most of PDF.js contributors have no stakes in supporting (integration or testing) other frameworks.

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

So, would it be ok to remove node-ensure and replace it with something else?

@yurydelendik
Copy link
Contributor Author

So, would it be ok to remove node-ensure and replace it with something else?

Only if will not regress webpack example

'use strict';

var useRequireEnsure = false;
if (typeof module !== 'undefined' && module.require) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about typeof window === 'undefined' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see conflict in node examples we have (due to DOM mock)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeof module !== 'undefined' && module.require can be changed to something else that will identify only node (preferably in less hacky way)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... maybe it's better to use System.import polyfill instead of require.ensure?
https://github.com/ModuleLoader/es6-module-loader
I believe, pdf.js is used more on browser side, so using ES6 modules seems more logical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using require.ensure is the only way to make it work for webpack, and require.ensure = require('node-ensure) is only way to make it work on node.js as well.

Adding a proper node.js detection (that will not be true for systemjs) will solve your issue

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

For some reason it provides (?) module and module.require object

Yes, System.js is a universal loader, so it provides all kinds of loaders, no matter which you choose. Apparently, require.ensure is not among loaders, supported by System.js

@yurydelendik
Copy link
Contributor Author

Yes, System.js is a universal loader,...

The PDF.js contributors might not be familiar with System.js -- can you create a small example to demonstrate the intended usage?

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

See #6791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants