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

Document the known differences of Electron's Node.js fork #26593

Open
3 tasks done
Prinzhorn opened this issue Nov 19, 2020 · 6 comments
Open
3 tasks done

Document the known differences of Electron's Node.js fork #26593

Prinzhorn opened this issue Nov 19, 2020 · 6 comments
Labels
documentation 📓 enhancement ✨ status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Nov 19, 2020

Mentioned in #26560, here's a separate issue to track it.

Preflight Checklist

  • I have read the Contributing Guidelines for this project.
  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Problem Description

#26560 was about the timeout option of the vm module not working in the main process. It looks like some of the Node.js tests are disabled. So I must assume there are some known differences between Electron and Node.js

Another thing I came across a few months ago is that Electron uses BoringSSL and not OpenSSL.

Proposed Solution

When searching for "electron node.js differences" I only found this page https://www.electronjs.org/docs/development/electron-vs-nwjs

What I propose is a similar page titled "Differences between Node.js and Electron's Node.js fork". This page will

  1. Establish that Electron even uses a fork and not vanilla Node.js (this is not obvious to everyone)
  2. List known issues with the fork linking to the GitHub issue (e.g. vm module timeout option not working in main process #26560)
  3. Known differences that are deliberate with a short explanation, e.g. the use of BoringSSL.

Additional Information

From the top of my head I've encounter at least these three:

  1. vm timeout (vm module timeout option not working in main process #26560)
  2. BoringSSL (Electron's use of BoringSSL #20204)
  3. Different ctrl+c handling than Node.js (Quit gracefully when Ctrl-C is pressed in console #5273)

I'll keep adding more as they come up:

@ckerr ckerr added enhancement ✨ status/confirmed A maintainer reproduced the bug or agreed with the feature labels Nov 19, 2020
@ckerr
Copy link
Member

ckerr commented Nov 19, 2020

I like this idea.

@MarshallOfSound
Copy link
Member

This makes sense kind of, but just to clarify a point or two.

Establish that Electron even uses a fork and not vanilla Node.js (this is not obvious to everyone)

Electron no longer has a fork of node, if you look at our DEPS file you'll see we directly depend on a tag in the nodejs/node repository on GitHub.

List known issues with the fork linking to the GitHub issue (e.g. #26560)

This is tricky, because I like to steer clear of a page that contains a list of known bugs. That's what the issue tracker is for, having a version controlled document for it just adds duplication. Probably better to just label certain issues with nodejs-divergence or something like that to indicate it's a bug that results in different behavior to NodeJS.

There are some things we can / should document though that aren't bugs but are intentional differences. For instance, the fact we use BoringSSL or that we disable NodeJS's ESM loader in the renderer process. And we're going to disable the vm module in the renderer process soon as well. (This is your point 3)

TLDR: Document design decisions that diverge us from node 👍 document all bugs that we have 👎

@Prinzhorn
Copy link
Contributor Author

Electron no longer has a fork of node, if you look at our DEPS file you'll see we directly depend on a tag in the nodejs/node repository on GitHub.

I'm not familiar with the internals of Electron. What is this folder for then? https://github.com/electron/electron/tree/46972abf8b949af80ea18b81df92ceeaa26ce00d/patches/node Maybe the terminology "fork" isn't accurate, but what's the practical difference between applying patches and actually maintaining a fork in git terminology? Electron patches Node.js, which can result in bugs and which results in known intentional differences. Or not?

This is tricky, because I like to steer clear of a page that contains a list of known bugs. That's what the issue tracker is for, having a version controlled document for it just adds duplication.

I absolutely agree. I was thinking more about bugs that are wontfix, because they are design decisions (like you suggested). The document could then link to a search for that tag, e.g. nodejs-divergence.

that aren't bugs but are intentional differences

This is like the fork terminology above but with the word bug.


Let me take a step back and make it very clear what my original intention was: someone new to Electron reads things like

Electron uses Chromium and Node.js so you can build your app with HTML, CSS, and JavaScript.

https://www.electronjs.org/

or

Electron exposes full access to Node.js API and its modules both in the Main and the Renderer processes.

https://www.electronjs.org/docs/tutorial/quick-start#nodejs-api

Now this person that is new to Electron starts working on their project. And eventually they stumble upon behavior they consider a bug. They run node index.js vs electron index.js with a simplified code to track down the issue (not using Electron APIs). They are absolutely puzzled why the same code with the same process.versions.node would do something different. It doesn't even occur to them that this is possible. They are not expected to know what magic Electron does to combine Node.js and Chromium. You as maintainers are blind to these kind of things, which is a completely natural process when being so involved in a project. For me as a newcomer I have no idea what is going on.

So all I'm asking for is to make it absolutely clear that these differences can occur, why they can occur and that some of these things cannot be fixed (these are bugs when you expect the same Node.js versions to do the same thing on the same machine).

Does that make sense?

@NotWearingPants
Copy link
Contributor

I'm already confused, but could you maybe also clarify how the Node.js event loop is intermingled with the Chrome and V8 event loops?
From testing, it seems that Promise tasks (from V8) are not being ran inside the Node.js event loop, even though it explicitly calls the V8 API to flush the event queue.

What sorcery did you do to make Node.js not aware that it is not controlling V8?

@codebytere
Copy link
Member

@NotWearingPants this talk deck might help a bit

@NotWearingPants
Copy link
Contributor

@codebytere Thanks! The slides were too vague but I found the video, and while it was a somewhat dumbed-down explanation what I got from it is that Electron spawns a separate thread which checks if Node.js's event loop is not empty, and then uses Chromium's PostTask to have the main thread process the events.
It seems that the relevant code is here:

task_runner_->PostTask(FROM_HERE, base::BindOnce(&NodeBindings::UvRunOnce,

While it is very cool and weird, I still don't get how both Node.js and Chromium think they are in charge of V8's event loop - there can be only one.
Also, this is one of the things that are important to document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📓 enhancement ✨ status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
None yet
Development

No branches or pull requests

6 participants