-
Notifications
You must be signed in to change notification settings - Fork 132
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
Net Fetch in Electron #3069
Net Fetch in Electron #3069
Conversation
@@ -3,13 +3,35 @@ | |||
"ignorePatterns": ["**/*"], | |||
"plugins": ["@nrwl/nx"], | |||
"overrides": [ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loosens up the eslint rules so I can @ts-ignore and use any
type.
I think this now has a working set of changes. I checked out this branch at commit f08a717, but before testing I reverted the changes in commit 8b8817c just because I don't yet have total faith in Then to check the "happy eyeballs" behavior is still intact, I did a cherry-pick of the changes from dfdc525 to make sure that the Zed lake service was listening only on That said, I do see that the CI flagged a bunch of breakages in |
fetch = | ||
process.env.JEST_WORKER_ID === undefined ? net.fetch : globalThis.fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned in a previous comment that despite the app functionality and e2e tests working ok after the changes thus far, we were still seeing many failures during yarn test
that looked like the following, such as in this CI run:
FAIL apps/zui/src/electron/start.test.ts
● app opens a window on startup
TypeError: Cannot read properties of undefined (reading 'fetch')
3 |
4 | export class ElectronZedLake extends Lake {
> 5 | fetch = net.fetch
| ^
6 | }
7 |
at new fetch (src/core/electron-zed-lake.ts:5:15)
at Function.boot (src/core/main/main-object.ts:47:18)
at Object.boot (src/electron/run-main/boot.ts:13:16)
at Object.main (src/electron/run-main/run-main.ts:19:19)
at Object.<anonymous> (src/electron/start.test.ts:18:20)
● activate creates window if there are none
Based on my understanding, these Jest tests are more lightweight so I assume the root cause is that Electron stuff like net.fetch
are simply not available when Jest tests are running. I figured one way to address this would be to determine if a test was running in Jest and, if so, choose a different fetch
. I did some web searches and saw others claiming success with checking Jest-ness via the presence of this JEST_WORKER_ID
env var so I used that approach here and it does seem to work, as yarn test
now passes locally and in CI and the e2e tests still pass. I'd appreciate some close scrutiny of my approach to confirm it's sound, though!
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
fetch = | ||
process.env.JEST_WORKER_ID === undefined ? net.fetch : globalThis.fetch |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
At commit 6626d66 (i.e., after the final two commits from @jameskerr in this branch), I just did a final looping run of the e2e test discussed in #3063:
That saw 130 successful runs and 0 failures before I stopped the loop. So indeed, these changes are very much having the intended effect! I then had a final chat with @jameskerr just to finalize my understanding of where things stand as of when this PR will merge. Going forward we'll depend on two different Fetch implementations:
Which is all to say that we never use the Fetch that's part of the Node.js that's bundled with Electron (despite it being the default), since we're always doing one of those two overrides. |
Override fetch using net.fetch in our electron application to allow for the Happy Eyeballs feature.
I've confirmed that the flakey test run reliably, but I need help confirming the happy eyeballs.
Fixes #3063