General review

[liach]

Requesting an overall review from @i509VCB before formal submission of this draft to the fabric api.

This draft is production-ready.

[i509VCB]

Gave it a good look through. Many of the concepts are actually quite interesting. Mainly a few questions is what this whole review consists of.

https://github.com/FabLabsMC/networking-api-v1-draft/blob/37b32cccd8cd754996bd25e25aa7e84eda0ca088/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/client/ClientLoginContext.java#L85-L96

Would it make sense to overload the void respond(PacketByteBuf buf); method into something like

default void respond(PacketByteBuf buf) {
    this.respond(buf, null);
}

Similarly below with the CompletableFuture methods as well?

https://github.com/FabLabsMC/networking-api-v1-draft/blob/37b32cccd8cd754996bd25e25aa7e84eda0ca088/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/client/ClientNetworking.java#L117-L125

Maybe have a way to check if the client's own play sender is available?

https://github.com/FabLabsMC/networking-api-v1-draft/blob/37b32cccd8cd754996bd25e25aa7e84eda0ca088/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/server/ServerLoginContext.java#L71-L74

This kinda seems like internals to me. Is there any reason this is exposed? If this is needed, maybe explain what the query id corresponds to more precisely.

https://github.com/FabLabsMC/networking-api-v1-draft/blob/37b32cccd8cd754996bd25e25aa7e84eda0ca088/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/server/ServerNetworking.java#L56-L58

Maybe say @see io.github.fablabsmc.fablabs.api.networking.v1.client.ClientNetworking for networking on the client. Or something similar if possible

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/PacketChannelCallback.java

Maybe move this and PacketListenerCallback to a callbacks package to clear up a bit of the root package?

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/PacketReceiver.java#L59-L61

Throw a /* @Nullable */ on the method so when we got annotations in fab api we can just search and replace those comments with the annotations?

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/PacketSender.java#L40-L46

Like the case above in ClientLoginConext, would it make sense to overload the method with no future listener to call the method with a future listener but pass null for the future.

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/impl/networking/client/ClientLoginNetworkAddon.java#L121-L124

I think there is a getClient call or would work with just an accessor to get the MinecraftClient from the ClientLoginNetworkHandler

Similar situation like above here is possible: https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/impl/networking/client/ClientPlayNetworkAddon.java#L119-L122

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/impl/networking/AbstractChanneledNetworkAddon.java#L148-L153

Maybe use Identifier#tryCreate here so you can just null check

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/mixin/networking/ClientConnectionMixin.java#L58

Mark this @Unique Repeast that with any fields you add to any mixins

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/resources/assets/networking-api-v1-draft/icon.png

Icon will obviously be changed when PRed to Fabric API

[liach]

1. For providing default impl for overloading, I'd argue that I will leave the implementation in the impl package than leaving default impl in the api package.
2. Client's own play sender is initialized the same time as client player's network handler, so that check you suggest would be extraneous.
3. The query id is like the "message id" in https://wiki.vg/Protocol#Login_Plugin_Request which can be totally internal and be left unexposed if desired
4. @see tag cannot include a description
5. Don't think a callback package with like 2 callbacks is very necessary. The root package is just a set of interfaces defined that expand from the Client/Server networking; they aren't entry to the API themselves, but part of the API.
6. Adding /*Nullable*/ sounds good
7. Again overloading goes to impl
8. Since we can call MinecraftClient.getInstance() don't think it worthes to add an extra accessor, and in a short while don't think minecraft can have two concurrent clients at once
9. Identifier.tryParse literally just throws that exception but eats that exception message, and I want that exception message
10. @Unique sure, dont' think it has a potency to clash though
11. Icon definitely subject to change

So a few changes:

• [x] Add /*Nullable*/ to packet receiver
• [x] Add @Unique to client connection field
• [x] Stop exposing query id for login queries, not much point
• [ ] Remember to change icon when it's part of a fabric pr

[i509VCB]

I'd say the @Unique isn't just for clashes but also acts as a sort of decorator telling maintainers that you added that field.