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

Merged
merged 13 commits into from
Jun 6, 2024
Merged

Net Fetch in Electron #3069

merged 13 commits into from
Jun 6, 2024

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented May 7, 2024

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.

@philrz
Copy link
Contributor

philrz commented Jun 6, 2024

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:

$ SUCCESSES=0; FAILURES=0; while true; do   yarn e2e -g pool-load-success.spec.ts --skip-nx-cache; if [ $? -eq 0 ] ; then SUCCESSES=$(expr $SUCCESSES + 1); else FAILURES=$(expr $FAILURES + 1); fi; echo "Successful test runs=$SUCCESSES, failed test runs=$FAILURES"; done

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:

  1. When running zed-node outside if the Zui app context, the Fetch that will be used is whichever comes in the Node.js that's being used to run zed-node.
  2. When running Zui, the Fetch that will be used is the "net Fetch" that's supplied by Chromium.

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.

@philrz philrz merged commit cc740ad into main Jun 6, 2024
4 checks passed
@philrz philrz deleted the net-fetch branch June 6, 2024 23:10
@philrz philrz linked an issue Jun 12, 2024 that may be closed by this pull request
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 Zed lake service launched by Zui is accepting remote connections
3 participants