WiseLibs / better-sqlite3

The fastest and simplest library for SQLite3 in Node.js.
MIT License
5.33k stars 393 forks source link

CI: Run test in Electron as well #1224

Open Prinzhorn opened 1 month ago

Prinzhorn commented 1 month ago

Looking at this https://github.com/WiseLibs/better-sqlite3/pull/1036#issuecomment-2227422329 I think it would be great if we could catch these things early and also if Electron specific PRs would actually show that tests are now passing.

Is this feasible? Too slow? It should be possible to run Jest with an Electron process and also run @electron/rebuild before running the tests.

@m4heshd you seem to be knowledgeable regarding GitHub CI stuff

mceachen commented 1 month ago

We’d want to run with both supported electron builds and a phase that pulls the latest beta that doesn’t mark the build as “failed.” That could be as simple as splitting the beta electron build into a different .yml file.

m4heshd commented 1 month ago

It would most probably slow down the tests significantly however, it's worth the effort.

I'm gonna have to look into how Mocha could handle this. The first thing that comes to my mind is to execute the Mocha process via Electron using ELECTRON_RUN_AS_NODE=1 env. Not sure if it would work as intended though. Need to run some tests. electron-mocha seems to be dedicated to doing exactly this.

Since it's not recommended to run DB tasks on the renderer process in the first place, I think it's unnecessary to test that scenario. So simply testing on the main process with ELECTRON_RUN_AS_NODE would do.

Prinzhorn commented 1 month ago

Since it's not recommended to run DB tasks on the renderer process in the first place, I think it's unnecessary to test that scenario.

Agreed, definitely only run tests in the main process

TheOneTheOnlyJJ commented 1 month ago

Given how common using better-sqlite3 with Electron is, this makes total sense and would improve the DX for many.

neoxpert commented 1 month ago

While it is not recommended to run (concurrent) DB tasks in the renderer, I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process. But in the end - if everything goes right - the renderer process should behave the same as the main process when it comes down to loading native node modules. At least until today I had no issues testing the better-sqlite3 integration with electron-mocha in the main and renderer process.

TheOneTheOnlyJJ commented 1 month ago

While it is not recommended to run (concurrent) DB tasks in the renderer, I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process. But in the end - if everything goes right - the renderer process should behave the same as the main process when it comes down to loading native node modules. At least until today I had no issues testing the better-sqlite3 integration with electron-mocha in the main and renderer process.

Given how this issue arose because things that were presumed to work ended up not working after a new electron version due to the lack of explicit tests, I believe not covering the greatest area possible with these new tests is an invitation for the same problems to appear further down the line. While not making sense to have separate tests as of now for both the main and renderer processes, a future electron version may introduce breaking changes that will require such tests to be written.

m4heshd commented 1 month ago

renderer process should behave the same as the main process when it comes down to loading native node modules.

It unfortunately doesn't. There's a countless amount of issues open in Electron about the very same issue and the team's response is always the same. They DO NOT recommend it. That's why they even unplugged the remote module and marked it as risky. If somebody utilizes Node's native features on the renderer, it's on them to maintain that unreliable mess. I've experienced this firsthand myself. This is also why Tauri went on a much safer path when it comes to that.

Prinzhorn commented 1 month ago

I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process

I use a Worker Thread spawned from main to host the entire application logic (to not block main). There is no reason to use better-sqlite3 in renderer. Ever. Using a hidden browser window might have been the recommended approach five years ago.

m4heshd commented 1 month ago

I use a Worker Thread spawned from main to host the entire application logic (to not block main). There is no reason to use better-sqlite3 in renderer. Ever. Using a hidden browser window might have been the recommended approach five years ago.

This and only this. I do not recommend any other practice when it comes to Electron. I learned an expensive lesson many years ago and this is the exact method I adapted to use. I even updated the threads documentation when I finally got it to be stable for production.

Final conclusion, DO NOT perform important logic on the renderer unless you're super lazy. Let it simply be the UI Renderer (which it's supposed to be) and a messenger.

mceachen commented 1 month ago

I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process

I use a Worker Thread spawned from main to host the entire application logic (to not block main). There is no reason to use better-sqlite3 in renderer. Ever. Using a hidden browser window might have been the recommended approach five years ago.

We should pin this to this project (maybe add it to the README?)