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

Add optional argument to PDFWorker for custom/bundled workers #7683

Closed
wants to merge 1 commit into from

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Oct 1, 2016

  • Support for webpack, browserify, etc users to pass in the proper web worker
    • E.g. webpack's worker-loader, browserify's webworkify

Meant as a prototype to fix #7612 . This isn't quite implemented as an overloaded constructor but instead with an optional argument.

An update to the wiki documenting how to use this functionality should be followed up.

- Support for webpack, browserify, etc users to pass in the proper web worker
  - E.g. webpack's worker-loader, browserify's webworkify
@timvandermeij
Copy link
Contributor

If this is accepted, we might need to update the examples in the /examples folder as well.

@yurydelendik
Copy link
Contributor

yurydelendik commented Oct 7, 2016

Thank you for request. It is really close to the desirable solution. The customWorker does not need to be tested, this also mean we cannot really fallback to fake worker (we will not know how to get to the js code to init that) -- all responsibility of giving proper port shall fall on the shoulders of the user/consumer. Let's rename customWorker to customPort and avoid all byte transfer testing -- just "jump" to the MessageHandler initialization. That said: a worker's port shall be ready to accept "GetDocRequest" without test/ready conversation.

@agilgur5
Copy link
Author

agilgur5 commented Oct 7, 2016

@yurydelendik doing that would require putting several if statements around the code and abstracting functions to be able to call them from two different places (or duplicating code), at which point it would indeed be easier to write a totally separate constructor. Several functions would still need to be abstracted out of the initialize code in order to be used in both constructors.

It took me a good hour just to figure that out and the several binds make it even more confusing what the proper scope should be if functions are abstracted out (not to mention I have no idea what to do with the various #if ENVIRONMENT directives)... I don't think I'd be able to write a decent PR to do that simply because the codebase is very difficult to modify without significant prior knowledge.


Correct me if I'm wrong, but it doesn't seem like the PR itself has any bugs in it, just that it could be optimized. My users and I are experiencing significant lag in reading PDFs due to #7612 and have been for the past 4 weeks (and I'm not the only one facing this bug), so I would sincerely appreciate if a fix could be released ASAP.

As such, do you think we could get this PR merged first as it fixes the major performance bug in #7612? I can make the changes to /examples as requested by @timvandermeij
Then you could make a separate PR to optimize that code at a later time; as I'm sure with your prior experience and deep understanding of the codebase that would take very little time (as opposed to me struggling with it for many hours on end).

Please let me know if that's possible and thank you for your understanding.

@agilgur5 agilgur5 mentioned this pull request Jan 12, 2017
@gdaolewe
Copy link

Any progress on this?

@jwahyoung
Copy link

jwahyoung commented Feb 22, 2017

+1 for this issue. Not sure why this wasn't designed this way anyway. I'm having an issue where I can't set the workerSrc at all because the requirejs configuration assumes a specific path. This isn't helpful at all.

The offending line is here:

workerSrc = requirejs.toUrl('pdfjs-dist/build/pdf.worker.js');
The problem is that it will overwrite any source that you specify, which results in a complete lack of developer control

@timvandermeij
Copy link
Contributor

Closing since it's superseded by #8107.

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.

Webpack instructions still cause 'fake worker' to load
5 participants