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

refactor(iroh)!: replace public fields in iroh client with accessors and use ref-cast to eliminate them entirely #2350

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Jun 6, 2024

Description

With 4 different clients, the current approach might be OK. But we are going to have more. E.g. gossip, see #2258 .

And in any case it feels weird to store the same thing multiple times.

So this replaces public fields in the iroh client with accessors and use ref-cast to eliminate them entirely. There is now only one rpc field, and you can switch from that to the different subsystem client views without runtime overhead, not even an arc clone.

Breaking Changes

Everything that uses the iroh client will have to switch from field accesses to accessor fns.

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@rklaehn rklaehn marked this pull request as ready for review June 7, 2024 10:40
@dignifiedquire
Copy link
Contributor

lets keep the fields and deprecate them in this PR, and drop them only in the next release

@rklaehn
Copy link
Contributor Author

rklaehn commented Jun 7, 2024

I asked on the public discord. For what it's worth, people seem to be in favour of this change:
image

@rklaehn rklaehn enabled auto-merge June 7, 2024 11:08
@rklaehn rklaehn added this pull request to the merge queue Jun 7, 2024
Merged via the queue into main with commit 35ce780 Jun 7, 2024
25 checks passed
@rklaehn rklaehn deleted the iroh-client-fields branch June 7, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants