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

navigator does not contain platform, therefore require('pdfjs-dist/build/pdf') leads to an error #15728

Closed
Sven3er opened this issue Nov 22, 2022 · 11 comments · Fixed by #17153
Closed

Comments

@Sven3er
Copy link

Sven3er commented Nov 22, 2022

Attach (recommended) or Link to PDF file here:

Configuration:

  • Web browser and its version:
    Node.js 18 .12.1
  • Operating system and its version:
    Microsoft Windows 11 Home 10.0.22621
  • PDF.js version:
    3.0.279
  • Is a browser extension:
    No
    Steps to reproduce the problem:
  1. install node js
  2. npm i pdfjs-dist
  3. npm i webex
  4. create any node js project and load "const webex = require('webex');" "const pdfjs = require('pdfjs-dist/build/pdf');"

What is the expected behavior? (add screenshot)
no error should occur

What went wrong? (add screenshot)
In line 3365 of the pdf.js file the object "navigator" does not contain the attribute "platform", therefore the following .include operations cause an error.

image

I would suggest changing line 3365 to 'const platform = typeof navigator.platform !== "undefined" ? navigator.platform : "";'
since not only the navigator object needs to exist.
The same should be done for legacy.

@Snuffleupagus
Copy link
Collaborator

The global navigator object isn't, to the best of my knowledge, available in Node.js environments; see also https://developer.mozilla.org/en-US/docs/Web/API/Navigator#browser_compatibility

Hence you must have an incomplete navigator polyfill somewhere in your code, which unfortunately means that it's a bug in your code rather than in the PDF.js library. As such, I don't think that we need/should make any changes here.

@Sven3er
Copy link
Author

Sven3er commented Nov 23, 2022

Hi,
I updated my original post since I still think checking if the "platform" attribute is part of the navigator before accessing it makes sense if you already check for the navigator object.

My code currently only looked like this:

const webex = require('webex');
const pdfjs = require('pdfjs-dist/build/pdf');

the Webex API seems to create the navigator object globally.
I can change the order of including the APIs or I could just write

navigator.platform = ''

after including the Webex API.
In my opinion, the PDF.js library should be bulletproof in this regard since javascript does not only run in node.js or in a browser. You never know what the environment you try to use PDF.js in comes up with and checking for "navigator.platform" instead of "navigator" does not come with a huge, if any, performance penalty.

@Snuffleupagus
Copy link
Collaborator

the Webex API seems to create the navigator object globally.

In that case, please file a bug with that project since it's actually responsible for this bug. (This issue just shows why incomplete polyfills are never a good idea...)

In my opinion, the PDF.js library should be bulletproof in this regard since javascript does not only run in node.js or in a browser.

At some point, you have to be able to assume that the environment isn't purposely misrepresenting its actual capabilities.

[...] and checking for "navigator.platform" instead of "navigator" does not come with a huge, if any, performance penalty.

Maybe not, but we have multiple call-sites using navigator that would all need to become more complicated and none of this is ever a problem in Firefox (which is the primary development target).


@calixteman Do you think that we should try and fix this, when it's clearly not our "fault" here.

@calixteman
Copy link
Contributor

Maybe we could just replace typeof navigator !== "undefined" ? navigator.platform : ""; by navigator?.platform || "".

@calixteman
Copy link
Contributor

For sure we can't support everything and especially every bug from other project: our main goal is to develop the Firefox pdf viewer, so @Sven3er you should really file a bug on Webex.
But as @Snuffleupagus said we've other use of navigator and we don't want to check if userAgent or platform is really a string or whatever..., polyfills are supposed to be there for this purpose, we have really enough work with our own bugs ;).
So I think it doesn't hurt that much to make the replacement I proposed above, but in general I'd say WONTFIX.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 23, 2022

Maybe we could just replace typeof navigator !== "undefined" ? navigator.platform : ""; by navigator?.platform || "".

Unfortunately that wouldn't work, since normally in Node.js the global navigator object isn't defined and trying to access a non-existent global object is a runtime error. The only generally safe thing would thus be:

typeof navigator !== "undefined" && typeof navigator?.platform === "string" ? navigator.platform : "";

@calixteman
Copy link
Contributor

Oh, you're right, so as said I'd say WONTFIX.

@shupingchu
Copy link

shupingchu commented Feb 7, 2023

Hi,

I am using React Native (0.66.4), and use webview to load pdf. But one of the library pdfjs-dist that I used in my project, throw the error TypeError: undefined is not an object (evaluating 'navigator.platform.includes'). I am wondering if can fix that by update the source code for

static get platform() {
    if (typeof navigator === "undefined") {
      return shadow(this, "platform", {
        isWin: false,
        isMac: false
      });
    }
    return shadow(this, "platform", {
      isWin: navigator.platform.includes("Win"),
      isMac: navigator.platform.includes("Mac")
    });
  }
static get platform() {
    if (typeof navigator === "undefined" || typeof navigator.platform === "undefined") {
      return shadow(this, "platform", {
        isWin: false,
        isMac: false
      });
    }
    return shadow(this, "platform", {
      isWin: navigator.platform.includes("Win"),
      isMac: navigator.platform.includes("Mac")
    });
  }

@dpitre
Copy link

dpitre commented Feb 8, 2023

Could it be the shim provider doesn't support it because it's deprecated?

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform

Maybe pdfjs shouldn't depend on deprecated props.

@Snuffleupagus
Copy link
Collaborator

Could it be the shim provider doesn't support it because it's deprecated?
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform
Maybe pdfjs shouldn't depend on deprecated props.

Well, that's currently the only thing that Firefox supports hence it's what we use. (Please keep in mind that the Firefox PDF Viewer is the primary development target here.)

As mentioned previously in this thread, we've multiple call-sites that access the navigator and all of them would need to become more complicated. (Which adds some overhead when e.g. working with and reviewing the relevant code.)


Given that this doesn't affect the built-in Firefox PDF Viewer, I cannot really justify spending my spare time trying to fix this (since I'm not getting paid to hack on the PDF.js library).
However if those affected by this would be willing to compensate me for my time, I don't believe that it'd be too difficult for me to provide a patch for this!

@aaroncrawford
Copy link

Node 21 now has a global navigator object that's an empty object now. So now anyone accessing the platform method is going to get a type error due to this line:

const platform = typeof navigator !== "undefined" ? navigator.platform : "";

The platform variable accesses includes on an object which will throw that type error.

Screenshot 2024-04-22 at 7 05 19 PM

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

Successfully merging a pull request may close this issue.

6 participants