Reorganisation of transport package

[oxisto]

Currently, the transport package holds all the different transport options web socket, HTTP and SSE. This has several issues for consumers of this library. Since go only does dependency pruning based on packages, all transitive libraries of all transports are "forwarded" to the consumer - even if he is never using the transport in the first place. Currently this only applies to the web socket library, but this might extend to others in the future.

My proposal would be to split up the transports into individual packages websocket, http, sse as sub packages within transport. This way, any eventual third-party dependencies only belonging to one particular transport are only forwarded to the consumer if he actually is using the transport. Plus, the transport package namespace is not "polluted" with different structs belonging to different things.

Is this something that you might be interested in? I have a demo implementation where I extracted the web socket one (https://github.com/oxisto/gqlgen/tree/reorg-transport) and could provide a PR if wanted.

[StevenACoffman]

I'm open to moving the transports to individual sub-packages, but I don't want to move the JSON decode to an internal package as you are doing, as I know of some people who are actively messing with that to inject alternative JSON implementations.

Also, despite the NewDefaultServer being deprecated, we need to keep it around as an example for others (and because a surprising number of people are still using it)

[oxisto]

I'm open to moving the transports to individual sub-packages, but I don't want to move the JSON decode to an internal package as you are doing, as I know of some people who are actively messing with that to inject alternative JSON implementations.

Thanks for the feedback! I am not quite sure what you mean with this, since at the moment the JSON decoding functions are unexported, so people can not use them outside of the transport package. Since they are used by the potential multiple sub-transport packages the only way to use them would be to move them to an internal package (or have them exported).

Also, despite the NewDefaultServer being deprecated, we need to keep it around as an example for others (and because a surprising number of people are still using it)

Unfortunately, this is a blocker then. Since NewDefaultServer uses all the transports, it would import all the sub-transport packages back into transport, rendering the effort mute.

[StevenACoffman]

There's a variety of unmerged PRs and maintained forks of gqlgen where the only difference is in the JSON decoding implementation. The problem was always that no one could agree on which alternative JSON implementation should be used for everyone. There is a proposal (but no PR) #2842 that we make JSON encoding/decoding be pluggable, so everyone can experiment with their own desired JSON implementation (especially with the work on http://github.com/go-json-experiment/json that eventually land in the stdlib as v2 "encoding/json" package). The proposed global variable / interface approach in that issue has the advantage that there is no gqlgen direct dependency on experimental JSON libraries. Moving things to "internal" moves away from that, and I don't want to discourage those people from contributing (since I would like them to contribute their maintenance efforts here rather than in isolated forks).

NewDefaultServer does not need to include websockets, SSE, or more exotic transports. It just needs to be an example of how people can configure gqlgen for themselves, and to facilitate a simple, dumb proof-of-concept trial of gqlgen, which is all some people ever need.

If we needed to, we could even move NewDefaultServer to a different package since it's marked deprecated. However, every time I make it so there is a non-zero change to update (even for something that is marked deprecated like NewDefaultServer), some people just don't bother to ever update, and then I get support requests for old versions for issues that have been solved for years. It just makes me leery of doing it

[oxisto]

There's a variety of unmerged PRs and maintained forks of gqlgen where the only difference is in the JSON decoding implementation. The problem was always that no one could agree on which alternative JSON implementation should be used for everyone. There is a proposal (but no PR) #2842 that we make JSON encoding/decoding be pluggable, so everyone can experiment with their own desired JSON implementation (especially with the work on http://github.com/go-json-experiment/json that eventually land in the stdlib as v2 "encoding/json" package). The proposed global variable / interface approach in that issue has the advantage that there is no gqlgen direct dependency on experimental JSON libraries. Moving things to "internal" moves away from that, and I don't want to discourage those people from contributing (since I would like them to contribute their maintenance efforts here rather than in isolated forks).

Ah got it. So it seems implementing this approach would be a prerequisite the. I can have a look into that if you want. We tried to implement a similar approach in jwt-go, which I co-maintain (see golang-jwt/jwt#301). We still could not get complete agreement on merging it however ;)

NewDefaultServer does not need to include websockets, SSE, or more exotic transports. It just needs to be an example of how people can configure gqlgen for themselves, and to facilitate a simple, dumb proof-of-concept trial of gqlgen, which is all some people ever need.

That sounds reasonable. A good compromise could be to first move the "exotic" transports out of the main transport package, as I did currently.

If we needed to, we could even move NewDefaultServer to a different package since it's marked deprecated. However, every time I make it so there is a non-zero change to update (even for something that is marked deprecated like NewDefaultServer), some people just don't bother to ever update, and then I get support requests for old versions for issues that have been solved for years. It just makes me leery of doing it

Yeah I understand.

[StevenACoffman]

I'm not saying that pluggable JSON encoding/decoding (with it defaulting to stdlib) would be a prerequisite for your desired transport reorganization, but it might be helpful to it, and they share certain similarities.

Certainly, if you are interested in implementing #2842 that would be very welcome!

Since you mentioned that you co-maintain golang-jwt/jwt, I have been looking for good public examples of combining Authorization (authz) schema directives with gqlgen and using tokens with roles and claims. I wrote up a high level survey (that if you have any feedback or corrections on I would also appreciate): https://dev.to/stevenacoffman/authorization-authz-and-graphql-3ag9

I get a lot of requests by people who are struggling with that combining rbac/abac schema directives and tokens.