Converted simpleviewer.js to await/async #14181
Conversation
There was a problem hiding this comment.
Unfortunately this breaks the example; please make sure that you actually test your patch before opening a pull request.
To test this locally:
- Run
gulp dist-install. - Run
gulp server. - Open http://localhost:8888/examples/components/simpleviewer.html in a browser.
Furthermore, I don't think that this will pass linting either; please remember to always run gulp lint locally and fix the reported errors.
Finally, please don't include unrelated changes in patches (i.e. the package-lock.json file).
|
@Snuffleupagus This is my first PR to pdf.js. Please review my PR. |
|
@Snuffleupagus Thank you for replying. I will check and look into it. |
|
|
Is there any errors printed in the console? |
|
@Snuffleupagus Please check my updated PR. Thank you. |
There was a problem hiding this comment.
You should not be removing the package-lock.json/package.json files from the repo, just from the patch itself.
Also, please follow https://github.com/mozilla/pdf.js/wiki/Squashing-Commits when addressing review comments.
examples/components/simpleviewer.js
Outdated
| loadingTask.promise.then(function (pdfDocument) { | ||
|
|
||
| (async function () { | ||
| const pdfDocument = await loadingTask.promise |
There was a problem hiding this comment.
This looks like a linting failure; please actually run gulp lint and fix all problems it reports.
There was a problem hiding this comment.
@Snuffleupagus I have run gulp lint and fixed the problem it reports. Please review it.
There was a problem hiding this comment.
Please make sure that you address all comments, since you've still incorrectly included the package-lock.json and package.json files in this patch!
(And make sure to clean-up the commit message during squashing, since sentences like "Changes requested in PR comments" and "Linting Changes requested in PR comments" are not meaningful there.)
There was a problem hiding this comment.
@Snuffleupagus I try but conflicts files don't remove and the other 4 commits are visible in my PR, I don't know about it. Kindly help me. Thanks.
There was a problem hiding this comment.
You should be able to use e.g. git rebase -i HEAD~5 and simply remove the commits that don't belong here.
(You may want to look-up a git tutorial, since basic knowledge of git is probably required in order to work efficiently with e.g. this project.)
There was a problem hiding this comment.
@Snuffleupagus Please check it.
There was a problem hiding this comment.
Once more, for probably the forth time, the patch is still including files that it shouldn't!
You must remove the package-lock.json and package.json from this patch, otherwise it'll never land.
Please don't ping me again until you've actually fixed all review comments (having to make the same comments over and over is just wasting my time).
There was a problem hiding this comment.
@Snuffleupagus Sorry for the inconvenience due to this PR. I have resolved the conflicts files. please check it.
Thank you.
PinRathod
force-pushed
the
simpleviewer_to_async
branch
9 times, most recently
from
86e57bd to
9df4c9b
Compare
There was a problem hiding this comment.
Your commit message has unrelated content, which doesn't make sense (i.e. the "This reverts commit c001b17." sentence).
examples/components/simpleviewer.js
Outdated
| @@ -92,10 +92,12 @@ const loadingTask = pdfjsLib.getDocument({ | |||
| cMapPacked: CMAP_PACKED, | |||
| enableXfa: ENABLE_XFA, | |||
| }); | |||
| loadingTask.promise.then(function (pdfDocument) { | |||
|
|
|||
There was a problem hiding this comment.
Unnecessary new-line, please revert.
There was a problem hiding this comment.
@Snuffleupagus Requested changes done. please check it. Thank you.
PinRathod
force-pushed
the
simpleviewer_to_async
branch
from
8930a95 to
4c463c6
Compare
|
@Snuffleupagus Thanks for your guidance. |
Closes #14128 Converted simpleviewer.js to await/async