Convert the files in the /web folder to ES6 modules
#8203
Conversation
web/app.js
Outdated
| @@ -41,6 +41,12 @@ import { SecondaryToolbar } from 'pdfjs-web/secondary_toolbar'; | |||
| import { Toolbar } from 'pdfjs-web/toolbar'; | |||
| import { ViewHistory } from 'pdfjs-web/view_history'; | |||
|
|
|||
| var { | |||
There was a problem hiding this comment.
In contrast to the commit message, this does not actually remove import * as pdfjsLib from 'pdfjs-web/pdfjs';. Why not, i.e., why is this particular instance being done this way instead of altering the * import like the other occurrences in this diff?
There was a problem hiding this comment.
Please note that https://github.com/mozilla/pdf.js/blob/master/web/debugger.js is currently being initialized with the entire pdfjsLib object, and I just didn't fancy re-factoring that code too in this already quite large PR.
Edit: But I suppose that I can attempt to do that if you insist :-)
There was a problem hiding this comment.
Good call. I didn't notice that, so I wondered why this was different. Looks good to me in that case.
There was a problem hiding this comment.
I looked through debugger.js quickly, and it seems that it only uses two of the pdfjsLib properties. Hence I've changed the initialization to only pass in those when calling PDFBug.init instead, and subsequently this point is now fixed :-)
web/pdf_thumbnail_viewer.js
Outdated
| var scrollIntoView = uiUtils.scrollIntoView; | ||
| var PDFThumbnailView = pdfThumbnailView.PDFThumbnailView; | ||
| import { getVisibleElements, scrollIntoView, watchScroll | ||
| } from 'pdfjs-web/ui_utils'; |
There was a problem hiding this comment.
I think this could be improved in terms of how we write multiline imports. Personally I would find the following easier to read:
import {
getVisibleElements, scrollIntoView, watchScroll
} from 'pdfjs-web/ui_utils';I don't know if there are actual conventions for this, but otherwise we should choose something that is the most readable and use it consistently. I think I just find the line starting with the indented curly bracket a bit strange.
This is a nit, but I figured I'd comment it now so we can make a decision (either this suggestion or your current version or something else) and stick with it for future PRs.
There was a problem hiding this comment.
I went back and forth a bit on the matter of formatting multi-line imports when writing the code.
In a way, I think it's more important that the exports are clear and readable, since ESLint will complain if you e.g. forget an import or import something that is unused.
However, if you think that readability is too much of an issue with the current import style, I can certainly change it as suggested.
There was a problem hiding this comment.
I would vote for the suggested notation because it makes the imports more readable in my opinion (somewhat similar to the object notation).
@yurydelendik Do you have a preference for a particular notation?
There was a problem hiding this comment.
Yes, but it might be complicated rule: if entire import including { and } can fit into single line then it's good as is, otherwise several lines first is import { and last is } from 'mod';
import { single } from 'first';
import {
one, two, THREE, four as fourOfSecond, etc
} from 'second';
There was a problem hiding this comment.
The style of the multi-line imports has been changed as requested, thank you both for the feedback!
|
The logical follow-up to this PR would be to convert the files in the For reference, in case it's useful with regards to asking for configuration advice and/or reporting bugs to SystemJS/Babel, here's a branch that converts everything in the |
|
Good point. While that should not block this PR, it should be investigated what is making things slow. Did you try to run it without Babel (since that performs transformations on the JavaScript)? Should we first release a new version before reviewing/merging this? |
That was suggested by Yury on IRC, and I completely concur that it's a good idea! |
…uilds, to avoid issues when converting the code to ES6 modules
Note that as discussed on IRC, this makes the viewer slightly slower to load *only* in `gulp server` mode, however the difference seem slight enough that I think it will be fine.
…ses to only specify the necessary imports Rather than always importing everything from the `web/pdfjs.js`, similar to all other imports we can just choose what we actually need.
|
/botio unittest |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/2713d751f9a85e3/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/da3240890777411/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/2713d751f9a85e3/output.txt Total script time: 2.59 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/da3240890777411/output.txt Total script time: 6.70 mins
|
From: Bot.io (Linux)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/44dbb32d89ae21b/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/44dbb32d89ae21b/output.txt Total script time: 2.75 mins Published |
|
Okay, let's try this. Thank you for the patch. |
Convert the files in the `/web` folder to ES6 modules
Please refer to the individual commit messages; careful testing will be necessary here.
As discussed on IRC, this makes the viewer slightly slower to load only in
gulp servermode, however the difference seem slight enough that I think it will be fine.Please Note: When looking at the second commit, ignoring whitespace by appending the
?w=1flag to the URL is probably necessary in order to be able to actually read the diff.This change is