-
Notifications
You must be signed in to change notification settings - Fork 10k
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 UMD header to viewer.js #7916
Conversation
Adding a UMD header to viewer.js allows using it as a real module in environments that support modular resource loading. Such as require.js and similar.
Could you fix the linting issues? After that, please squash the commits into one commit. Refer to https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for how to do that easily. |
@timvandermeij, thank you for stepping in. Seems I can't figure out, why the checks are failing. The most recent failure was on |
I think https://github.com/mozilla/pdf.js/pull/7916/files?w=1#diff-dd42e9b2d2a652b8e312efa567698aa6R22 should be |
@timvandermeij, the |
@timvandermeij, I work with the repository through a web browser. I don't have |
@timvandermeij, looking at GitHub documentation I can see there is an option to squash commits upon merging PR's by repository maintainers (https://github.com/blog/2141-squash-your-commits). |
@Snuffleupagus, understood, thank you. I added indentation, as most of the code is now a function body, thus should be indented one more level. Do you suggest not to indent, even it is a function body now? Can you please let me use your code you've tested the PR with? I assume it is a single HTML with a PDF constellation. But I would like to have the exact environment you checked the PR with, making sure I miss nothing. |
@Snuffleupagus, you've just removed your change request. Does it mean you'll proceed wit h the code as is now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, it seems that my review comment vanished, hence I'm re-submitting it below.
I work with the repository through a web browser. I don't have git installed. Can I still squash the commits from the web interface?
Please follow the work-flow described in https://github.com/mozilla/pdf.js/wiki/Contributing, see also https://github.com/mozilla/pdf.js#getting-the-code, since this allows you to actually test the changes made in the PR locally, to ensure that it works as intended!
Having just tested the PR locally, using gulp server
(as described in the links above), I'm sorry to have to tell you that in its current form this PR does not actually work.
Hence this needs to be addressed, before we can test/review this PR properly.
Furthermore, please revert all the indentation changes made in the PR. We want to avoid unnecessarily changing the blame on every single line in this file. (This is consistent with the adding of UMD headers in all other files, which is the only case where not indenting is acceptable.)
Finally, as outlined in https://github.com/mozilla/pdf.js/wiki/Squashing-Commits, please squash the commits; we want to avoid a PR landing with "broken" intermediate commits.
Since you'll need to follow the contribution guidelines linked above anyway, in order to test the PR, squashing shouldn't be a problem.
Removing excessive indentation, as requested by mozilla#7916 (review).
The concrete example is needed how it is/will be used. I have a feeling that this PR is incomplete solution and will be needing further and bigger chunk of work to be published at pdfjs-dist. At the moment app.js is better entry point and already has all needed API to create a custom viewer. |
@yurydelendik, the general idea is that you'd use a module loader, such as One of drawbacks of the current state of I'll try to change the test files as well to use |
I understand what you are trying to do, but I don't see who and how will benefit from the change. Please provide example that will consume the patched viewer.js and how it's different from just loading non-patched viewer.js. |
@Snuffleupagus, reverted the indentation and fixed @yurydelendik, I hope you'd agree. Thank you in advance. |
@Snuffleupagus, re-worked the global export of |
Agree with above, plus I'm adding few of my own: I'm blocking this PR with the items above. |
@yurydelendik, thank you for the review. I can think about yet another commit that will solve the enhancement №4: "refactor UI to not use document.getElementById or similar queries from global page context". I could make the The only tricky part here, is the very different nature of I would like to hear your opinion, please. |
You can just use web/app as "main" module for your app and use your choice of packager to produce your own bundle. |
That said, I probably would like to see pdf_viewer.js to contain all the classes including app.js but this effort is blocked by the items mentioned above |
@yurydelendik, this PR already resolves items: 1, 2, and 3. But I don't feel myself confident enough in PDF.js codebase to also resolve items 4 and 5. Would be glad for any help, though. |
Closing for now since there has been no more progress in a few months and we're moving away from UMD headers in favor of ES6 modules soon anyway (see https://github.com/mozilla/pdf.js/projects/3). |
Adding a UMD header to viewer.js allows using it as a real module in environments that support modular resource loading. Such as require.js and similar.