Convert examples/learning/helloworld.html to await/async #14138

cathydev · 2021-10-14T06:17:37Z

Issue this pull request solves:

Snuffleupagus

Besides the inline comments, please improve the commit message to say what you're actually changing since it's not entirely clear otherwise without looking at the code; e.g. "Convert examples/learning/helloworld.html to use async/await".

Snuffleupagus · 2021-10-14T07:07:12Z

examples/learning/helloworld.html

@@ -1,3 +1,4 @@
+


Unrelated change, please revert.

Snuffleupagus · 2021-10-14T07:09:46Z

examples/learning/helloworld.html

-      // Support HiDPI-screens.
-      var outputScale = window.devicePixelRatio || 1;
+  (async () => {
+  const pdf = await loadingTask;


Sorry, but this doesn't actually work!
The loadingTask is not a Promise, but rather a structure with a promise-property; see

pdf.js/src/display/api.js

Lines 527 to 601 in f6d9d91

/**

* The loading task controls the operations required to load a PDF document

* (such as network requests) and provides a way to listen for completion,

* after which individual pages can be rendered.

*/

class PDFDocumentLoadingTask {

static get idCounters() {

return shadow(this, "idCounters", { doc: 0 });

}

constructor() {

this._capability = createPromiseCapability();

this._transport = null;

this._worker = null;

/**

* Unique identifier for the document loading task.

* @type {string}

*/

this.docId = `d${PDFDocumentLoadingTask.idCounters.doc++}`;

/**

* Whether the loading task is destroyed or not.

* @type {boolean}

*/

this.destroyed = false;

/**

* Callback to request a password if a wrong or no password was provided.

* The callback receives two parameters: a function that should be called

* with the new password, and a reason (see {@link PasswordResponses}).

* @type {function}

*/

this.onPassword = null;

/**

* Callback to be able to monitor the loading progress of the PDF file

* (necessary to implement e.g. a loading bar).

* The callback receives an {@link OnProgressParameters} argument.

* @type {function}

*/

this.onProgress = null;

/**

* Callback for when an unsupported feature is used in the PDF document.

* The callback receives an {@link UNSUPPORTED_FEATURES} argument.

* @type {function}

*/

this.onUnsupportedFeature = null;

}

/**

* Promise for document loading task completion.

* @type {Promise<PDFDocumentProxy>}

*/

get promise() {

return this._capability.promise;

}

/**

* Abort all network requests and destroy the worker.

* @returns {Promise<void>} A promise that is resolved when destruction is

* completed.

*/

async destroy() {

this.destroyed = true;

await this._transport?.destroy();

this._transport = null;

if (this._worker) {

this._worker.destroy();

this._worker = null;

}

}

}

Hence the correct change would be const pdf = await loadingTask.promise;.

Furthermore, given that the example cannot possibly work as-is: please make sure to (in this case manually) test the changes that you make to ensure that your code actually works.
Testing this example locally, after making changes, includes:

Running gulp dist-install.

Running gulp server.

Opening http://localhost:8888/examples/learning/helloworld.html in a browser and check that the example loads and works correctly without e.g. any errors printed in the console.

Snuffleupagus · 2021-10-14T07:10:51Z

examples/learning/helloworld.html

+  const pdf = await loadingTask;
+  //
+  // Fetch the first page
+  //
+  const page = await pdf.getPage(1);
+  var scale = 1.5;
+  var viewport = page.getViewport({ scale });
+  // Support HiDPI-screens.
+  var outputScale = window.devicePixelRatio || 1;
+
+  //
+  // Prepare canvas using PDF page dimensions
+  //
+  var canvas = document.getElementById("the-canvas");
+  var context = canvas.getContext("2d");

-      //
-      // Prepare canvas using PDF page dimensions
-      //
-      var canvas = document.getElementById('the-canvas');
-      var context = canvas.getContext('2d');
+  canvas.width = Math.floor(viewport.width * outputScale);
+  canvas.height = Math.floor(viewport.height * outputScale);
+  canvas.style.width = Math.floor(viewport.width) + "px";
+  canvas.style.height = Math.floor(viewport.height) + "px";

-      canvas.width = Math.floor(viewport.width * outputScale);
-      canvas.height = Math.floor(viewport.height * outputScale);
-      canvas.style.width = Math.floor(viewport.width) + "px";
-      canvas.style.height =  Math.floor(viewport.height) + "px";
+  var transform =
+    outputScale !== 1 ? [outputScale, 0, 0, outputScale, 0, 0] : null;

-      var transform = outputScale !== 1
-        ? [outputScale, 0, 0, outputScale, 0, 0]
-        : null;
+  //
+  // Render PDF page into canvas context
+  //
+  var renderContext = {
+    canvasContext: context,
+    transform,
+    viewport,
+  };
+  page.render(renderContext);


Note that this entire block of code should be indented with two more spaces.

cathydev · 2021-10-14T08:12:51Z

@Snuffleupagus Hi! Thank you so much for the feedback. I made all the changes you requested but there's an error in the console that says "Uncaught ReferenceError: pdfjsLib is not defined" and I tried with the master branch but it's printing the same error. I went to interactive examples and changed the fiddle file with my code and it's working
https://jsfiddle.net/Lu59de7m/

Snuffleupagus · 2021-10-14T08:38:54Z

I made all the changes you requested but there's an error in the console that says "Uncaught ReferenceError: pdfjsLib is not defined" and I tried with the master branch but it's printing the same error.

That might suggest that you didn't follow all of the steps outlined at the bottom of #14138 (comment), since it should definitely work when run against the master branch.

Please also see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits, since we don't use separate fixup commits in this project.

Snuffleupagus · 2021-10-14T14:48:30Z

examples/learning/helloworld.html

+    var transform =
+      outputScale !== 1 ? [outputScale, 0, 0, outputScale, 0, 0] 
+      : null;


There's a trailing space added at the end of a line above, and let's keep the existing formatting; i.e.

var transform = outputScale !== 1 ? [outputScale, 0, 0, outputScale, 0, 0] : null;

Finally, please remember https://github.com/mozilla/pdf.js/wiki/Squashing-Commits

cathydev · 2021-10-14T17:22:52Z

Thanks, I had a little problem with npm install and that was the reason why of the error but everything is working now

Snuffleupagus

This looks better now, but there's still a few small things to fix here.
Also, in the commit message the same sentence is repeated more than once; please ensure that the final commit message reads only "Convert examples/learning/helloworld.html to await/async".

Snuffleupagus · 2021-10-14T17:28:56Z

examples/learning/helloworld.html

+    const transform =
+      outputScale !== 1 
+      ? [outputScale, 0, 0, outputScale, 0, 0] 
+      : null;


As mentioned in #14138 (comment), please ensure that this reads as follows (and without any trailing spaces):

const transform = outputScale !== 1 ? [outputScale, 0, 0, outputScale, 0, 0] : null;

Snuffleupagus · 2021-10-14T17:29:31Z

examples/learning/helloworld.html

@@ -75,4 +76,4 @@ <h2>JavaScript code:</h2>
      document.getElementById('script').text;
 </script>
 </body>
-</html>
+</html>


Please revert this change, i.e. re-add the line-break at the end here.

Snuffleupagus · 2021-10-14T17:30:10Z

package.json

+  "license": "Apache-2.0",
+  "dependencies": {
+    "pdfjs-dist": "file:build/dist"
+  }


Please revert all changes to the package-lock.json and package.json files.

Snuffleupagus · 2021-10-14T17:30:53Z

examples/learning/helloworld.html

-  loadingTask.promise.then(function(pdf) {
+  const loadingTask = pdfjsLib.getDocument(url);
+  (async () => {
+    const pdf = await loadingTask.promise;;


One semi-colon too many here.

Snuffleupagus

r=me; thanks for the patch!

cathydev changed the title ~~Convert examples/learning/helloworld.html to await/async #14121~~ Convert examples/learning/helloworld.html to await/async Oct 14, 2021

Snuffleupagus requested changes Oct 14, 2021

View reviewed changes

Snuffleupagus reviewed Oct 14, 2021

View reviewed changes

cathydev force-pushed the await/async-issue branch from 3133970 to 8c82ed6 Compare October 14, 2021 17:20

Snuffleupagus requested changes Oct 14, 2021

View reviewed changes

Convert examples/learning/helloworld.html to await/async

47f60a7

cathydev force-pushed the await/async-issue branch from 8c82ed6 to 47f60a7 Compare October 14, 2021 18:20

Snuffleupagus approved these changes Oct 14, 2021

View reviewed changes

Snuffleupagus merged commit 4b8496e into mozilla:master Oct 14, 2021

cathydev deleted the await/async-issue branch October 15, 2021 01:44

timvandermeij added the other label Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert examples/learning/helloworld.html to await/async #14138

Convert examples/learning/helloworld.html to await/async #14138

cathydev commented Oct 14, 2021 •

edited

Loading

Snuffleupagus left a comment

Snuffleupagus Oct 14, 2021

Snuffleupagus Oct 14, 2021 •

edited

Loading

Snuffleupagus Oct 14, 2021

cathydev commented Oct 14, 2021

Snuffleupagus commented Oct 14, 2021

Snuffleupagus Oct 14, 2021

cathydev commented Oct 14, 2021

Snuffleupagus left a comment

Snuffleupagus Oct 14, 2021

Snuffleupagus Oct 14, 2021

Snuffleupagus Oct 14, 2021

Snuffleupagus Oct 14, 2021

Snuffleupagus left a comment

	/**
	* The loading task controls the operations required to load a PDF document
	* (such as network requests) and provides a way to listen for completion,
	* after which individual pages can be rendered.
	*/
	class PDFDocumentLoadingTask {
	static get idCounters() {
	return shadow(this, "idCounters", { doc: 0 });
	}

	constructor() {
	this._capability = createPromiseCapability();
	this._transport = null;
	this._worker = null;

	/**
	* Unique identifier for the document loading task.
	* @type {string}
	*/
	this.docId = `d${PDFDocumentLoadingTask.idCounters.doc++}`;

	/**
	* Whether the loading task is destroyed or not.
	* @type {boolean}
	*/
	this.destroyed = false;

	/**
	* Callback to request a password if a wrong or no password was provided.
	* The callback receives two parameters: a function that should be called
	* with the new password, and a reason (see {@link PasswordResponses}).
	* @type {function}
	*/
	this.onPassword = null;

	/**
	* Callback to be able to monitor the loading progress of the PDF file
	* (necessary to implement e.g. a loading bar).
	* The callback receives an {@link OnProgressParameters} argument.
	* @type {function}
	*/
	this.onProgress = null;

	/**
	* Callback for when an unsupported feature is used in the PDF document.
	* The callback receives an {@link UNSUPPORTED_FEATURES} argument.
	* @type {function}
	*/
	this.onUnsupportedFeature = null;
	}

	/**
	* Promise for document loading task completion.
	* @type {Promise<PDFDocumentProxy>}
	*/
	get promise() {
	return this._capability.promise;
	}

	/**
	* Abort all network requests and destroy the worker.
	* @returns {Promise<void>} A promise that is resolved when destruction is
	* completed.
	*/
	async destroy() {
	this.destroyed = true;
	await this._transport?.destroy();

	this._transport = null;
	if (this._worker) {
	this._worker.destroy();
	this._worker = null;
	}
	}
	}

Convert examples/learning/helloworld.html to await/async #14138

Convert examples/learning/helloworld.html to await/async #14138

Conversation

cathydev commented Oct 14, 2021 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

cathydev commented Oct 14, 2021

Snuffleupagus commented Oct 14, 2021

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

cathydev commented Oct 14, 2021

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2021

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

cathydev commented Oct 14, 2021 •

edited

Loading

Snuffleupagus Oct 14, 2021 •

edited

Loading