apollographql / rover

The CLI for Apollo GraphOS
https://rover.apollo.dev
Other
407 stars 85 forks source link

Consider allowing globs in `file` inputs for `subgraph check` and `subgraph publish` #694

Open EverlastingBugstopper opened 3 years ago

EverlastingBugstopper commented 3 years ago

Description

trevor-scheer commented 3 years ago

Small nitpick but I think a rename (or alias, non-breaking) file -> files would make more sense since we'll be globbing a potentially >1 number of files.

abernix commented 3 years ago

I think we should carefully define the desired behavior for how subgraph schema files are merged. Is merely concatenation acceptable? If so, then there is a short-term, already supported answer of (if I recall):

cat **/*.graphql | rover subgraph publish -

However, given that buildFederatedSchema does more than just concatenate distinct typeDefs members that are passed to it, I think there's some investigation to be had there to succeed in meeting the user's expectations?

To elaborate, buildFederatedSchema does this which subsequently calls modulesFromSDL before eventually calling buildSchemaFromSDL.

That's to say, if I recall, this was an explicit thing that we deferred in the initial Rover milestone because it necessitated much more of a standard library that could do those things. It's not that it's doing anything particularly complicated, but it's more than just concatenation.

If what we want today is concatenation, we may want to consider merely suggesting cat **/*.graphql | rover ... until we have the resources to do more (both underlying library and human-time to design, review and build).

abernix commented 3 years ago

Btw, acknowledge that the cat 🐈 work-around would not work for YAML-based configuration. I think we just want to decide if we're okay with the initial version of this just being concatenation or if that's in any way problematic.

If it's not a problem, then let's do it and then build the more complete merging stuff later, but we should make sure that doesn't result in a federated schema of a different shape than the one available at runtime via a schema produced with buildFederatedSchema.

prasek commented 3 years ago

@abernix @EverlastingBugstopper consistent behavior with buildFederatedSchema and the result of rover subgraph introspect would be ideal. Is that already part of harmonizer?

abernix commented 3 years ago

There is no relation from harmonizer to either rover subgraph introspect or buildFederatedSchema.

The workflow with the best UX for using modules (that is, sets of typeDefs and resolvers) with Federation right now is to use buildFederatedSchema and run rover subgraph introspect against that. This leverages buildFederatedSchema (and thus modulesFromSDL and friends) via runtime introspection to the _Service type.

Because of constraints / limitations to what we can do with GraphQL in Rover today, if the module functionality within subgraphs (e.g., splitting subgraph types across files) is desired with functionality which is more than just concatenation, I suggest we rally around rover subgraph introspect. Alternatively, while admittedly more limited, I think the current approach to pipe in a stream of files sets the right expectations (i.e., that it's merely concatenation than anything more principled!)

EverlastingBugstopper commented 3 years ago

Sounds like this can be closed?

EverlastingBugstopper commented 3 years ago

This seems to actually be a duplicate issue regardless, closing this and I'll link to it in the other issue.

mdecurtins commented 2 years ago

Just coming here to say this would be very useful. We don't have a federated schema, but for graph publish it would be great to supply a file glob instead of being limited to a local endpoint or a single *.graphql file. Interesting workaround using cat, but we all know that official support is better than workarounds. 😄