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

Net Fetch in Electron #3069

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Net Fetch in Electron #3069

wants to merge 11 commits into from

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented May 7, 2024

Add back node-fetch to the zed-node client for now.
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

@@ -3,13 +3,35 @@
"ignorePatterns": ["**/*"],
"plugins": ["@nrwl/nx"],
"overrides": [
{
Copy link
Member Author

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.

@philrz philrz requested review from nwt and philrz May 7, 2024 19:46
@philrz
Copy link
Contributor

philrz commented May 7, 2024

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 why-is-node-running and didn't need to debug in this test. With that I was able to run the looping repro script through 100 iterations and saw zero failures. 🎉

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 localhost:9867 and confirmed I was still able to successfully load data, so that also checks out. 👍

That said, I do see that the CI flagged a bunch of breakages in yarn test caused by the most recent commit and I was able to reproduce that locally, so I guess those Jest tests need some kind of treatment to run ok in this new config. @jameskerr hopefully that's easy? 🤞

Comment on lines 5 to 6
fetch =
process.env.JEST_WORKER_ID === undefined ? net.fetch : globalThis.fetch
Copy link
Contributor

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.

Comment on lines 5 to 6
fetch =
process.env.JEST_WORKER_ID === undefined ? net.fetch : globalThis.fetch

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e2e pool-load-success test hanging on close
3 participants