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

[experimental] Figure out recommended way to override fetch dispatcher in Node #2126

Closed
steveluscher opened this issue Feb 15, 2024 · 2 comments · Fixed by #2178
Closed

[experimental] Figure out recommended way to override fetch dispatcher in Node #2126

steveluscher opened this issue Feb 15, 2024 · 2 comments · Fixed by #2178
Assignees
Labels
enhancement New feature or request released

Comments

@steveluscher
Copy link
Collaborator

The current version of @solana/web3.js lets people supply their own HTTPAgent (often at their own peril) to be able to put a keep-alive config on the wire. We'll need a way to do that in the new web3.js without bothering people who use it on the web / React Native.

Options:

  1. Create a transport for use in Node only. Plumb dispatcher all the way down to fetch(). See typedef.
  2. Recommend that people override the global Unidici dispatcher on which fetch() depends. See this Stack Overflow question.
@steveluscher steveluscher added the enhancement New feature or request label Feb 15, 2024
@steveluscher steveluscher self-assigned this Feb 22, 2024
steveluscher added a commit that referenced this issue Feb 27, 2024
…h` (#2178)

# Summary

The current version of web3.js allows folks to supply what's called an ‘HTTP Agent.’ This is a class that mediates how network requests should be fetched. One example is to establish a connection pool for a given URL, and keep connections open for a time so that TLS/SSL negotiation doesn't have to be done on every request.

The new web3.js up until this point has punted on how to reintroduce this concept. In this PR we bring it back in the form of allowing callers to create transports with custom Undici `Dispatchers`.

It only works in Node. If you supply the `dispatcher_NODE_ONLY` property in non-Node environments you'll get a console warning that your config has been ignored.

# Alternate designs considered

I desperately wanted to avoid the situation in this PR, which was to create a config property on transports that's only usable in Node.

Alternate approaches considered:

1. Create a special transport for use in Node. Callers would have to `createDefaultNodeRpcTransport()` and/or `solanaRpcFactoryForNode()` which I thought was a bridge too far.
2. Encourage people to [inject a global dispatcher](https://stackoverflow.com/a/77526783/802047). This is a terrible idea for all of the reasons that global state is terrible:
    * Applies to all connections in the process
    * Your global dispatcher can be unset by someone _else_
    * There might already be a global dispatcher installed (eg. a `ProxyAgent`) and you might unknowingly replace it
3. Export a `Dispatcher` setter that lets you install a dispatcher accessible _only_ to `@solana/fetch-impl`. Besides having most of the bad features of #​2, this would not work. We inline `@solana/fetch-impl` in the build, meaning there's no longer anywhere to export such a method from.

# Test Plan

```shell
cd packages/fetch-impl/
pnpm test:unit:node
cd ../rpc-transport-http/
pnpm test:unit:node
```

See benchmark tests in next PR.

Closes #2126.
Copy link
Contributor

github-actions bot commented Mar 2, 2024

🎉 This issue has been resolved in version 1.90.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Because there has been no activity on this issue for 7 days since it was closed, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant