fix: electron main and renderer process detection

[GuoSirius]

Fixes #1877

[robertsLando]

@axi92 could you take a look at this to be sure it doesn't break you?

Ref: #1856

[GuoSirius]

https://www.electronjs.org/docs/latest/api/process#processtype-readonly

[axi92]

@axi92 could you take a look at this to be sure it doesn't break you?

Ref: #1856

I can check later today. Thanks for mentioning me!

Maybe somebody could implement a test with playwright? https://www.electronjs.org/docs/latest/tutorial/automated-testing#using-playwright
I have never done a test with the electron context but maybe it's possible to have a fully automated test that checks if mqtt is still working with the latest electron version.

[robertsLando]

@axi92 Here we have wtr (who uses playwright under the hoods) but I have no clue how to configure it with electron... IMO the easier way is to simply create a super simple electron app that connects to a local mqtt server

[axi92]

npm run build returns errors.
@GuoSirius were you able to build your code?

Errors

$ npm run build

> [email protected] build
> npm run build:ts && npm run build:browser


> [email protected] build:ts
> rimraf build/ && tsc -p tsconfig.build.json

src/lib/is-browser.ts:5:16 - error TS2339: Property 'type' does not exist on type 'Process'.

5    if (process.type === 'renderer') return true
                 ~~~~

src/lib/is-browser.ts:6:28 - error TS2339: Property 'electron' does not exist on type 'Process'.

6    else if (typeof process.electron !== 'undefined') return false
                             ~~~~~~~~


Found 2 errors in the same file, starting at: src/lib/is-browser.ts:5

[GuoSirius]

@axi92 It's all right now

[robertsLando]

Fix lint issues: npm run lint-fix

[axi92]

I checked the current branch and it breaks for me.
It uses ws instead of mqtt in the browser context of electron.

What was your vision of an simple electron app?
An automated test or a test that is manually executed in an environment with a display?

[robertsLando]

@axi92 An automated test of course with a dedicated GH workflow like for browser tests

[axi92]

@robertsLando I created #1883 as a poc that electron tests are possible.

[GuoSirius]

In fact, this test should not be placed here for judgment.

Because the rendering process is also a browser environment, since it is a browser environment, it should go the browser logic.

Otherwise, the ws module may be mistakenly loaded in the browser environment of the rendering process, and the ws module cannot run in the browser environment.

If you do need to use the mqtt protocol in an electron application, it is recommended to put the connection establishment logic in the main process, and then use the interprocess communication mechanism to interact between the row rendering process and the main process.

The rendering process must have a window and a document, but not necessarily a Node.js environment, depending on the configuration at instantiation time.

With the main rendering process's responsibilities clearly divided, the browser environment's detection mechanism can maintain the previous standard judgment logic.

[robertsLando]

Lint is failing, I think you have to exclude electron-tests directory from tsconfig

[robertsLando]

If you do need to use the mqtt protocol in an electron application, it is recommended to put the connection establishment logic in the main process, and then use the interprocess communication mechanism to interact between the row rendering process and the main process.

I'm not an electron user but I think it makes sense. Also mqtt-explorer (an electron application that uses mqtt) seems to do it this way

https://github.com/thomasnordquist/MQTT-Explorer/tree/f539e03c7ebc44ab274159ba370b14fcd269b414?tab=readme-ov-file#develop

[axi92]

If you use electron in its best practice mode that is the case, yes I agree with you.

On the other hand if you do ignore that on purpose because you choose to, I don't know if it is a good option to deny the user that option.

The workaround we used before there was electron support was to patch mqtt after npm i

[robertsLando]

@axi92 I'm not blocking this, I just would like to be sure it works for all scenarios :)

[GuoSirius]

The Electron main process owns all Node environments and can establish connections according to the logic of non-browser environments.

The rendering process belongs to the browser environment and should follow the logic of the browser environment to establish the connection.

Just because the rendering process has more process, Node, and other accessible environments than the normal browser environment, it should not be attributed to the Node environment.

In this cross-end application, it is up to the user to decide which way to establish the connection. According to the needs, the connection can be established in the main process or the rendering process, rather than the application library to force detection to determine the protocol.

According to the above convention, the is-browser.ts file is purely used to detect the browser environment, regardless of whether there are other factors, such as the main process or the rendering process.

[robertsLando]

@GuoSirius I understand, so is there a way to allow both you and @axi92 being able to use mqtt with electron as you want? I don't want to break anyone I just would like to allow everybody using this lib as they prefer, maybe a way to override the is_browser check could work? with an env var maybe. Giving that your solution seems the most correct one and I agree with you we could use yours to detect the electron rendering process and consider it browser and in order to also allow @axi92 using it like he want add an env var to allow users forcing the value of is_browser. What do you think?

cc @axi92

[axi92]

Override is a good option! Either with options or an ENV var as long as it is possible to override the check I am fine with it 👍

[robertsLando]

Ok so @GuoSirius would you like to add the override with the env var to this PR please?

[robertsLando]

Could this be closed now that we have forceNativeWebSocket option?