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

Use globalThis for webpack's output.globalObject instead of this #14915

Closed
tamuratak opened this issue May 14, 2022 · 5 comments · Fixed by #14966
Closed

Use globalThis for webpack's output.globalObject instead of this #14915

tamuratak opened this issue May 14, 2022 · 5 comments · Fixed by #14966
Labels

Comments

@tamuratak
Copy link
Contributor

tamuratak commented May 14, 2022

Use globalThis for webpack's output.globalObject instead of this. In the current configuration, this is used.

pdf.js/gulpfile.js

Lines 232 to 233 in 96b125f

// Required to expose e.g., the `window` object.
output.globalObject = "this";

As you know, globalThis exists for this use case. Comparing both compatibility tables of MDN and PDF.js, it seems feasible to use globalThis.

Advantages

One of advantages of using globalThis is that it allows us to import pdf.js from modules. Let us consider the following example:

index.html:

<script src="main.js" type="module"></script>

main.js:

import './pdf.js'
console.log(pdfjsLib)

With the current this configuration, we see an error since the top level this will be undefined in Modules.

Uncaught TypeError: root is undefined
  • Web browser and its version: Firefox 100.0
  • Operating system and its version: macOS Big Sur 11.6.5
  • PDF.js version: the current master 96b125f

With the globalThis configuration, we see no error:

@Snuffleupagus
Copy link
Collaborator

https://webpack.js.org/configuration/output/#outputglobalobject

That resource contains the following sentence: To make UMD build available on both browsers and Node.js, set output.globalObject option to 'this'. which applies to the PDF.js builds since the gulpfile uses libraryTarget: "umd", and our GENERIC builds must be usable in both browser and Node.js environments.

Hence the (obvious) question: Would your proposed changes work correctly everywhere?

  • First of all, does the entire PDF.js test-suite (i.e. gulp test) work correctly with your proposed changes?
  • Secondly, does gulp ci-test and all of the Node.js examples still work correctly with your proposed changes?

@tamuratak
Copy link
Contributor Author

First of all, does the entire PDF.js test-suite (i.e. gulp test) work correctly with your proposed changes?

No. However, even at master, it seems that gulp test fail on some tests.

On other tests,

$  node -v
v16.15.0

The following are the diffs of generated files:

--- ../tmp/pdfjs_master/build/generic/build/pdf.js	2022-05-15 12:32:13.000000000 +0900
+++ build/generic/build/pdf.js	2022-05-15 12:47:09.000000000 +0900
@@ -29,7 +29,7 @@
 		exports["pdfjs-dist/build/pdf"] = factory();
 	else
 		root["pdfjs-dist/build/pdf"] = root.pdfjsLib = factory();
-})(this, () => {
+})(globalThis, () => {
 return /******/ (() => { // webpackBootstrap
 /******/ 	"use strict";
 /******/ 	var __webpack_modules__ = ([
@@ -1355,7 +1355,7 @@
 
   const workerId = await worker.messageHandler.sendWithPromise("GetDocRequest", {
     docId,
-    apiVersion: '2.14.300',
+    apiVersion: '2.14.301',
     source: {
       data: source.data,
       url: source.url,
@@ -3488,9 +3488,9 @@
 
 }
 
-const version = '2.14.300';
+const version = '2.14.301';
 exports.version = version;
-const build = '96b125fb7';
+const build = '2643143d9';
 exports.build = build;
 
 /***/ }),
@@ -16387,8 +16387,8 @@
 
 var _xfa_layer = __w_pdfjs_require__(22);
 
-const pdfjsVersion = '2.14.300';
-const pdfjsBuild = '96b125fb7';
+const pdfjsVersion = '2.14.301';
+const pdfjsBuild = '2643143d9';
 {
   if (_is_node.isNodeJS) {
     const {
--- ../tmp/pdfjs_master/build/generic/build/pdf.worker.js	2022-05-15 12:32:14.000000000 +0900
+++ build/generic/build/pdf.worker.js	2022-05-15 12:47:11.000000000 +0900
@@ -29,7 +29,7 @@
 		exports["pdfjs-dist/build/pdf.worker"] = factory();
 	else
 		root["pdfjs-dist/build/pdf.worker"] = root.pdfjsWorker = factory();
-})(this, () => {
+})(globalThis, () => {
 return /******/ (() => { // webpackBootstrap
 /******/ 	"use strict";
 /******/ 	var __webpack_modules__ = ([
@@ -117,7 +117,7 @@
     const WorkerTasks = [];
     const verbosity = (0, _util.getVerbosityLevel)();
     const apiVersion = docParams.apiVersion;
-    const workerVersion = '2.14.300';
+    const workerVersion = '2.14.301';
 
     if (apiVersion !== workerVersion) {
       throw new Error(`The API version "${apiVersion}" does not match ` + `the Worker version "${workerVersion}".`);
@@ -74420,8 +74420,8 @@
 
 var _worker = __w_pdfjs_require__(1);
 
-const pdfjsVersion = '2.14.300';
-const pdfjsBuild = '96b125fb7';
+const pdfjsVersion = '2.14.301';
+const pdfjsBuild = '2643143d9';
 })();
 
 /******/ 	return __webpack_exports__;
--- ../tmp/pdfjs_master/build/generic-legacy/build/pdf.js	2022-05-15 12:32:26.000000000 +0900
+++ build/generic-legacy/build/pdf.js	2022-05-15 12:47:22.000000000 +0900
@@ -29,7 +29,7 @@
 		exports["pdfjs-dist/build/pdf"] = factory();
 	else
 		root["pdfjs-dist/build/pdf"] = root.pdfjsLib = factory();
-})(this, () => {
+})(globalThis, () => {
 return /******/ (() => { // webpackBootstrap
 /******/ 	var __webpack_modules__ = ([
 /* 0 */,
@@ -5796,7 +5796,7 @@
             _context7.next = 5;
             return worker.messageHandler.sendWithPromise("GetDocRequest", {
               docId: docId,
-              apiVersion: '2.14.300',
+              apiVersion: '2.14.301',
               source: {
                 data: source.data,
                 url: source.url,
@@ -8651,9 +8651,9 @@
   writable: true,
   value: new WeakSet()
 };
-var version = '2.14.300';
+var version = '2.14.301';
 exports.version = version;
-var build = '96b125fb7';
+var build = '2643143d9';
 exports.build = build;
 
 /***/ }),
@@ -25326,8 +25326,8 @@
 
 var _xfa_layer = __w_pdfjs_require__(175);
 
-var pdfjsVersion = '2.14.300';
-var pdfjsBuild = '96b125fb7';
+var pdfjsVersion = '2.14.301';
+var pdfjsBuild = '2643143d9';
 {
   if (_is_node.isNodeJS) {
     var _require = __w_pdfjs_require__(178),
--- ../tmp/pdfjs_master/build/generic-legacy/build/pdf.worker.js	2022-05-15 12:32:26.000000000 +0900
+++ build/generic-legacy/build/pdf.worker.js	2022-05-15 12:47:22.000000000 +0900
@@ -29,7 +29,7 @@
 		exports["pdfjs-dist/build/pdf.worker"] = factory();
 	else
 		root["pdfjs-dist/build/pdf.worker"] = root.pdfjsWorker = factory();
-})(this, () => {
+})(globalThis, () => {
 return /******/ (() => { // webpackBootstrap
 /******/ 	var __webpack_modules__ = ([
 /* 0 */,
@@ -165,7 +165,7 @@
       var WorkerTasks = [];
       var verbosity = (0, _util.getVerbosityLevel)();
       var apiVersion = docParams.apiVersion;
-      var workerVersion = '2.14.300';
+      var workerVersion = '2.14.301';
 
       if (apiVersion !== workerVersion) {
         throw new Error("The API version \"".concat(apiVersion, "\" does not match ") + "the Worker version \"".concat(workerVersion, "\"."));
@@ -93149,8 +93149,8 @@
 
 var _worker = __w_pdfjs_require__(1);
 
-var pdfjsVersion = '2.14.300';
-var pdfjsBuild = '96b125fb7';
+var pdfjsVersion = '2.14.301';
+var pdfjsBuild = '2643143d9';
 })();
 
 /******/ 	return __webpack_exports__;

@Snuffleupagus
Copy link
Collaborator

No. However, even at master, it seems that gulp test fail on some tests.

Assuming that this is limited to intermittently failing reference tests, since some browsers/platforms unfortunately suffer from those, a small number of "failures" are usually fine.


Given that everything apparently works, I suppose that we could implement the change suggested in this issue.

@Snuffleupagus
Copy link
Collaborator

Given that everything apparently works, I suppose that we could implement the change suggested in this issue.

Please note that this will however require that someone affected by this submits a patch to implement it :-)
@tamuratak Do you perhaps want to open a PR?

@tamuratak
Copy link
Contributor Author

Thank you for your clarification. I will open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants