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

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

Open
philrz opened this issue May 10, 2024 · 2 comments
Open

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

philrz opened this issue May 10, 2024 · 2 comments
Assignees

Comments

@philrz
Copy link
Contributor

philrz commented May 10, 2024

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 #3063 (comment), evidence suggests that nodejs/undici#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 Net Fetch in Electron #3069
  2. Put back the changes in Native Node Fetch #3061
  3. Do a looping test using the repro steps in e2e pool-load-success test hanging on close #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 philrz self-assigned this May 10, 2024
@philrz
Copy link
Contributor Author

philrz commented Jun 6, 2024

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 nodejs/undici#2026 bug. See #3069 (comment) for details.

Closing this issue.

@philrz philrz closed this as not planned Won't fix, can't repro, duplicate, stale Jun 6, 2024
@philrz philrz reopened this Jun 20, 2024
@philrz
Copy link
Contributor Author

philrz commented Jul 9, 2024

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.

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

No branches or pull requests

1 participant