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

Support Systemjs #6791

Closed
Vanuan opened this issue Dec 22, 2015 · 34 comments
Closed

Support Systemjs #6791

Vanuan opened this issue Dec 22, 2015 · 34 comments
Labels

Comments

@Vanuan
Copy link

Vanuan commented Dec 22, 2015

Please add support for systemjs.

@yurydelendik
Copy link
Contributor

I would request more information since no examples are provided. And I think Browserify works with pdfjs-dist currently (with some modules exceptions).

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Here's the most common example to use systemjs:

npm install jspm
jspm init # use defaults
jspm install pdfjs-dist

index.html

<!DOCTYPE html>
<html lang="en-us">

<head>
<meta charset="utf-8">
<title>Title</title>
</head>

<body>
    <script src="jspm_packages/system.js"></script>
    <script src="config.js"></script>
    <script>
      System.import('src/app.js');
    </script>
</body>
</html>

src/app.js

# ES6 syntax
import pdfjs from 'pdfjs-dist';

# expected pdfjs to be a window.PDFJS object

@yurydelendik
Copy link
Contributor

Thank you for example

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

In previous versions we could use jspm shim to use pdfjs.
https://github.com/jspm/registry/wiki/Configuring-Packages-for-jspm
But after inclusion of node-ensure, we can't (node-ensure doesn't seem to be compatible with system.js).

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Actually, this is a request for jspm packager, not specifically Systemjs loader.
I.e. it would be fine if it would require jspm shim, but don't add incompatible changes like node-ensure.

@yurydelendik
Copy link
Contributor

@Vanuan

Having the following in the config.js workaround the issue:

    "npm:pdfjs-dist@1.3.122": {
      "process": "github:jspm/nodelibs-process@0.1.2",
      "node-ensure": "empty",
      "entry?name=[hash]-worker.js": "empty"
    },

(empty is a empty.js file with no content)

Similar hack is need for Browserify

@yurydelendik
Copy link
Contributor

Now issue: pdf.worker.js is loaded twice, that's an issue was solved by request.ensure() in webpack. The './pdf.worker.js' has to be forbidden from loading until it's really needed.

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

I thought "PDFJS.disableWorker = true;" solves that? Isn't it?

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

If you use System.import('./pdf.worker'), it will issue only one http request.

Or even System.import('pdfjs-dist/build/pdf.worker')

Is there a specific reason why node-ensure was used? I believe System loader works fine in Webpack.

@yurydelendik
Copy link
Contributor

"PDFJS.disableWorker = true;" is not an option,since we want to move PDF processing on the different thread. We don't want suggest lousy solution for library users, do we?

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Well, I'd be prefer lousy solution than no solution at all. For my use case, even 1 second page freeze is fine.
I'd be glad if you find a better solution.

@yurydelendik
Copy link
Contributor

Is there a specific reason why node-ensure was used?

If you look carefully at what examples/webpack/ produces:

pdf.js yury$ ls -la build/webpack/
total 2384
drwxr-xr-x   5 yury  staff     170 Dec 22 15:20 .
drwxr-xr-x  34 yury  staff    1156 Dec 22 10:08 ..
-rw-r--r--   1 yury  staff  546125 Dec 21 16:35 1.bundle.js
-rw-r--r--   1 yury  staff  546463 Dec 21 16:35 9d074593b165291f150e-worker.js
-rw-r--r--   1 yury  staff  122796 Dec 21 16:35 bundle.js

"bundle.js" is main file that loads (that means it will bring and initialize UI after 122796 bytes -- faster page start up). "9d074593b165291f150e-worker.js" is a worker which will run the logic and don't lock UI on slow devices. "1.bundle.js" portion is loaded only if disableWorker triggered and for legacy browsers such as IE9.

@yurydelendik
Copy link
Contributor

I'd be glad if you find a better solution.

I'm expecting the systemjs might have some kinda guard to not parse all requires (replacing "./pdf.worker.js" with "empty" did not work for me).

@Vanuan Can you file an issue with systemjs to see if they have something better in mind? Thanks.

@yurydelendik
Copy link
Contributor

Sounds like the very last issue at systemjs systemjs/systemjs#983 ?

@yurydelendik
Copy link
Contributor

Well, I'd be prefer lousy solution than no solution at all. For my use case, even 1 second page freeze is fine.

@Vanuan, nothing changed in usage of the pdf.combined.js file (I hope) -- looks like we need to keep it for such cases. You need to use it as you described as an issue in #6729. From your description it was not clear you are planing to use systemjs to load PDFJS.

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

nothing changed in usage of the pdf.combined.js file

Ah, great!

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Anyway, something has definitely changed with regard to using it with worker.
Previously, we could use full path to worker, but get Uncaught ReferenceError: require is not defined inside it:

PDFJS.workerSrc = 'jspm_packages/npm/pdfjs-dist@1.3.120/build/pdf.worker.js';

Now, it seems like it's stuck on loading files. As if module loaded callback never completes.
The last module requested is process.

Probably, Systemjs can't process the following construction:

(function(process) {
  if (typeof PDFJS === 'undefined') {
    (typeof window !== 'undefined' ? window : this).PDFJS = {};
  }
})(require('process'));

@yurydelendik
Copy link
Contributor

Anyway, something has definitely changed with regard to using it with worker.
Previously, we could use full path to worker,

pdf.combined.js fully included/includes pdf.worker.js and does not need workerSrc to be specified.

@yurydelendik
Copy link
Contributor

Changes to pdf.combined.js mozilla/pdfjs-dist@a7cd5f7#diff-eccf5b94e31b0939738de07167e02af6

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Yeap, I'm just evaluating options of using workers along with jspm.

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Now issue: pdf.worker.js is loaded twice, that's an issue was solved by request.ensure() in webpack

So, now it is being loaded in the main thread and doesn't use importScripts()?

@yurydelendik
Copy link
Contributor

My src/app.js is:

import pdfjs from 'pdfjs-dist';

console.log(PDFJS);

So it's not even using getDocument yet, but in network monitor it shows two entries for pdf.worker.js

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

It's strange. I only see one entry.

@yurydelendik
Copy link
Contributor

It's strange. I only see one entry.

It shall be none AFAIK :)

@yurydelendik
Copy link
Contributor

The same on Firefox and Chrome:
screen shot 2015-12-22 at 5 34 42 pm

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Ah. So looks like Systemjs dumbly parses all requires and tries to load them...

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

Conditional require is not a very common use case.

@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

That's why it tries to load node-ensure too.

@Vanuan Vanuan changed the title Support Systemjs and Browserify Support Systemjs Dec 22, 2015
@Vanuan
Copy link
Author

Vanuan commented Dec 22, 2015

@yurydelendik
How such requires help webpack? Webpack still loads worker using importScripts, right?

@yurydelendik
Copy link
Contributor

Conditional require is not a very common use case.

I would disagree, systemjs/systemjs#983 and https://github.com/webpack/docs/wiki/code-splitting are common use cases.

Webpack still loads worker using importScripts, right?

It does. via new Worker(..). But it also has an option to load pdf.worker.js as module if workers are disabled.

@yurydelendik
Copy link
Contributor

The issue will be solved by #6775 -- the systemjs has auto-detect for type of the module. Now it's commonjs, with UMD it will be AMD so it will not try to parse require().

P.S. wip at https://github.com/yurydelendik/pdf.js/tree/pdfjsumd

@Vanuan
Copy link
Author

Vanuan commented Dec 28, 2015

Wow, great, I'll try as soon as merged and released.

@timvandermeij
Copy link
Contributor

Closing as fixed by #6825. If there are remaining problems, please open a new issue.

@yurydelendik
Copy link
Contributor

I also recommend to override module format when using jspm: jspm install npm:pdfjs-dist -o "{format: 'amd'}"

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

No branches or pull requests

3 participants