isNodeJS breaks on Electron 'main' process which really IS NodeJS

[burtonator]

the isNodeJS code is broken on Electron in the 'main' process.

The problem is that in Electron there are two processes. The 'main' and the 'renderer' ... the 'main's is really just node. The 'renderer' is sort of a browser with some 'require' semantics.

I assume this code was added to work within the renderer process but it accidentally broke the 'main' process.

Here's the code in question:

is_node.js:

const isNodeJS =
  typeof process === "object" &&
  process + "" === "[object process]" &&
  !process.versions["nw"] &&
  !process.versions["electron"];

... I think this can be fixed by just adding another check to:

typeof window !== 'undefined' ... when process.versions["electron"]...

it might be best to have a cleaner function that has 'electron_process_type' of 'none' , 'renderer' or 'main'

... I'm can submit a PR for this later once I verify a fix...

Attach (recommended) or Link to PDF file here: n/a ... breaks on all of them.

Configuration:

• Web browser and its version: Electron 5.x (n/a)
• Operating system and its version: MacOS Catalina
• PDF.js version: 2.4.456
• Is a browser extension: no

Steps to reproduce the problem:

1. Try to use PDF.js from Electron main process

[burtonator]

Any objection to a PR with this? It's a little functiony but it's clear and easy to read.

function computeElectronProcessType() {
  if (process && process.versions && process.versions["electron"]) {
    if (typeof window !== "undefined") {
      return "renderer";
    }

    return "main";
  }

  return "none";
}

const electronProcessType = computeElectronProcessType();

// NW.js / Electron is a browser context, but copies some Node.js objects; see
// http://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/#access-nodejs-and-nwjs-api-in-browser-context
// https://electronjs.org/docs/api/process#processversionselectron
const isNodeJS =
  typeof process === "object" &&
  process + "" === "[object process]" &&
  !process.versions["nw"] &&
  electronProcessType !== "renderer";

export { isNodeJS };

[burtonator]

... and I don't have a fix for NW

[Snuffleupagus]

Any objection to a PR with this? It's a little functiony but it's clear and easy to read.

Well, it seems very unfortunate to add dependencies on what essentially looks like internal Electron-state to the PDF.js code-base. If something simple like e.g. an inline typeof window !== "undefined" check works, then that's probably preferred here.

[burtonator]

@Snuffleupagus internal Electron state? It's just a check to see if the window was there. I'm not calling any electron functions. I just moved this to a function so it was a big more readable / maintainable. It's not dependent on any electron state.

[Snuffleupagus]

The problem is that in Electron there are two processes. The 'main' and the 'renderer' ... the 'main's is really just node. The 'renderer' is sort of a browser with some 'require' semantics.

Based on the above, I don't think it should be entirely surprising if someone unfamiliar with Electron would assume that the quoted names refer to some kind of "internal" Electron state.

It's just a check to see if the window was there.

Again, then please just add a simple typeof window check as necessary :-)

I just moved this to a function so it was a big more readable / maintainable. It's not dependent on any electron state.

By introducing a bunch of (what appears to be) arbitrary strings, i.e. "renderer"/"main"/"none", that would actually make the code much more difficult to read/maintain since anyone looking at it wouldn't really know what they are meant to represent.

[timvandermeij]

I would agree with the above. If a simple typeof window addition will suffice, that is absolutely preferred and a PR for that is welcome. The isNode function already handles quite a few cases and we want to limit complexity here, especially since the Node.js feature detection is not too easy given the number of exceptions.

[matthopson]

FWIW, I think I'm running into a similar issue. When upgrading from v2.3.200, our Cypress tests that invoke pdfjs-dist will fail. It's taken me a little bit to narrow down the issue, but it eventually led me here looking for electron related errors. The code being executed shouldn't run in a browser context, but I think it's trying to when run with Cypress (the test runner is an Electron app, so process.versions.electron is present).

[Snuffleupagus]

Fixed by PR #12085, perhaps?

[timvandermeij]

Yes, this should be fixed now.

[leohxj]

I use pdf-parse module instead of pdfjs in main process to parse metainfo of PDF document.

[ava-cassiopeia]

I'm not sure if I should open a new bug or comment on this one, but I'm still experiencing this exact same issue using Electron 11.2.0 and pdfjs-dist 2.5.207.

When I open up pdfjs-dist/es5/build/pdf.js and throw in some logging statements to print out the value of isNodeJS it shows that the value is false, despite being run in the main process of an Electron app.

---

If it's relevant, my main.js for electron is available here. It is called by running:

node_modules/.bin/electron src/electron/js/main.js

[ava-cassiopeia]

Ah, I commented too soon. A quick search for 12085 (this fixing PR's ID) on https://github.com/mozilla/pdf.js/releases reveals that the fix rolls up into v2.6.347, which is currently a pre-release. Please excuse the noise, I'll figure out how to integrate that version into my build.