search

brimdata / zui

Zui is a powerful desktop application for exploring and working with data. The official front-end to the Zed lake.
https://www.brimdata.io/download/
Other
1.8k stars 132 forks source link

Use Native Node fetch (when/if possible) #3071

Open philrz opened 5 months ago

philrz commented 5 months ago

At first there was celebration when the changes in #3061 allowed us to drop the node-fetch dependency. Unfortunately we learned after the fact that the change caused intermittent CI failures as described in #3063. The fix for that problem came in #3069 where we switched over to using Electron's net.fetch in the app and going back to node-fetch elsewhere.

As described in https://github.com/brimdata/zui/issues/3063#issuecomment-2099167661, evidence suggests that https://github.com/nodejs/undici/issues/2026 might have been the root cause of the symptoms described in #3063, but the fix is only available in Node v21 and newer, and Electron's current latest release is still using Node v20, and based on prior history it looks like we're probably several months away from a release that will include a Node release new enough to include the fix.

This issue serves as a reminder for us to revisit the topic when a new Electron release is available to test with. The rough summary of the steps would be to create a branch where we:

  1. Back out the changes in #3069
  2. Put back the changes in #3061
  3. Do a looping test using the repro steps in #3063 to confirm still we don't have hangs
  4. If that's all successful, merge that branch to main, which will once again achieve the goal of dropping node-fetch
philrz commented 5 months ago

It turns out we were able to drop node-fetch dependency by the time we merged #3069 and no longer rely on the Fetch implementation that has the https://github.com/nodejs/undici/issues/2026 bug. See https://github.com/brimdata/zui/pull/3069#issuecomment-2153536647 for details.

Closing this issue.

philrz commented 3 months ago

We ended up reopening this one after all. The perf problem cited in #3105 led us to bring node-fetch back into the picture again via the changes in #3106.

That brings us back to the previous plan of record: Once there's an Electron release that's bundled with a Node v21 or newer, I can check if our CI runs reliably when using the fetch implementation in that Node, and if that's true and it also performs as well as what we're getting right now with node-fetch, we could once again drop the node-fetch dependency.