[desktop] Flow type electron
ClosedPublic
Actions

Authored by michal on Jan 5 2023, 5:01 AM.

Details

Reviewers

tomek
ginsu
inka
ashoat

Commits

rCOMM8e680d7716fe: [desktop] Flow type electron

Summary

ENG-2221
This diff adds flow types for the electron package (or at least for the parts we use).

The types are mostly taken from the documentation but I been also looking at the typescript types (which are generated automatically from the documentation anyway).

Some modules are only available in the main process (main.js) or in the renderer process (preload.js). To make that explicit I re-export the appropriate types in electron/main and electron/renderer modules. I use them instead of the electron module which contains all types.

Test Plan

Run flow in desktop.

Diff Detail

Repository

rCOMM Comm

Branch

michal/electron-flow

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Jan 5 2023, 5:01 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptJan 5 2023, 5:01 AM

michal added a parent revision: D6175: [desktop] Type bridge object between desktop and web.Jan 5 2023, 5:02 AM

Harbormaster completed remote builds in B15119: Diff 20614.Jan 5 2023, 5:15 AM

michal requested review of this revision.Jan 5 2023, 5:15 AM

ashoat added inline comments.Jan 5 2023, 8:25 AM

desktop/flow-typed/npm/electron_vx.x.x.js
2–3 ↗	(On Diff #20614)	Should these lines be kept? In particular, the version might be important? Probably not a big deal...
desktop/src/main.js
9 ↗	(On Diff #20614)	From talking to @michal, apparently Electron has some magic where it ignores the `/main` part here. This is surprising and inconsistent with standard Node / ESM import behavior. I checked the NPM package and in fact there is no `/main` or `/renderer`. Two questions: Why are the namespaces necessary? Could we just type the module as it is used, without changing anything other than the types? Can you link any documentation explaining this behavior from Electron? (Where the `/main` is ignored.)

Back to you with questions

This revision now requires changes to proceed.Jan 5 2023, 8:29 AM

Keep comments for flow-typed generated stub

Harbormaster failed remote builds in B15156: Diff 20667!Jan 9 2023, 3:32 AM

Responding to @ashoat questions:

Why are the namespaces necessary? Could we just type the module as it is used, without changing anything other than the types?

The "namespaces" aren't really necessary. We could just import { stuff } from 'electron' in both main.js and preload.js. The problem is that some things are only available on the main process (that runs main.js) and some only on the renderer process (that runs preload.js and our web app code). For example, if you tried to run import { ipcMain } from 'electron' in the preload.js there would a runtime error. So I was just trying to catch this error at compile time.

Can you link any documentation explaining this behavior from Electron? (Where the /main is ignored.)

The best thing I can find is the typescript declarations. You can see here that they also declare modules with /main and /renderer: LINK (NOTE: this file is just copied from comm/node_modules/electron because I couldn't find it directly on GitHub, it's automatically generated)

Separately: I don't think the android CI failure is because of my changes?

In D6176#185686, @michal wrote:

Responding to @ashoat questions:

Why are the namespaces necessary? Could we just type the module as it is used, without changing anything other than the types?

The "namespaces" aren't really necessary. We could just import { stuff } from 'electron' in both main.js and preload.js. The problem is that some things are only available on the main process (that runs main.js) and some only on the renderer process (that runs preload.js and our web app code). For example, if you tried to run import { ipcMain } from 'electron' in the preload.js there would a runtime error. So I was just trying to catch this error at compile time.

Can you link any documentation explaining this behavior from Electron? (Where the /main is ignored.)

The best thing I can find is the typescript declarations. You can see here that they also declare modules with /main and /renderer: LINK (NOTE: this file is just copied from comm/node_modules/electron because I couldn't find it directly on GitHub, it's automatically generated)

Thanks for explaining. My view on this is that it would have been better to put this change in a separate diff, and explain that you were doing it to catch errors statically instead of at runtime. Given that TypeScript does this, we're probaly safe to do the same thing.

Separately: I don't think the android CI failure is because of my changes?

From time-to-time we see intermittent Android CI failures. The first thing to try in this case is to restart the job (I just restarted it). If that still doesn't solve the issue, you may need to rebase on master. Can you try to make sure the CI is green before landing?

This revision is now accepted and ready to land.Jan 9 2023, 8:30 AM

Change types of objects described as "modules" in the electron documentation, from class to type. This matches their behaviour better (their methods aren't binded).

Harbormaster completed remote builds in B15254: Diff 20794.Jan 11 2023, 7:03 AM

Closed by commit rCOMM8e680d7716fe: [desktop] Flow type electron (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyJan 11 2023, 7:11 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM8e680d7716fe: [desktop] Flow type electron.