apollographql / apollo-ios

📱  A strongly-typed, caching GraphQL client for iOS, written in Swift.
https://www.apollographql.com/docs/ios/
MIT License
3.89k stars 728 forks source link

Codegen Option: Optional Exported Imports #3322

Closed Mordil closed 8 months ago

Mordil commented 10 months ago

Use case

Generated operations files are always @_exported import Apollo API and it's causing conflicts throughout our codebase with Object and Realm's definition of Object, and our GraphQL Scalar Date with Foundation.Date

In general, it's a mixed bag of using @_exported import statements due to the nature of

  1. It forces exposure of the entire module
  2. In some cases, it breaks things like DocC documentation from properly resolving symbols & building
  3. It's an underscored attribute that Apple has stated several times should not be used as wide-spread as it is

Describe the solution you'd like

The codegen output should be updated much like the operations were for marking them as final https://github.com/apollographql/apollo-ios/pull/3189/files

AnthonyMDev commented 9 months ago

Thanks for the feedback @Mordil. Without the @exported import the generated models become pretty unwieldy. It means that you basically need to do add an import ApolloAPI in most of the places where you are using the models. So we don't think that creating yet another configuration option that allows you to disable this is really a good idea.

Though we do agree that ApolloAPI is exposing too much right now, which creates the type conflicts you're seeing and pollutes code completion. The solution we hope to implement for that that is using the @_spi attribute. This should allow us to only expose publicly the narrowly scoped parts of ApolloAPI that are helpful for consumers, while hiding the rest behind the SPI.

Because @_spi is still an underscored attribute, we've put off doing this, in hopes that its implementation would be finalized sooner than later. But I think we should probably move this up on our list of priorities and go ahead with the underscored attribute.

Additionally, we are currently working on this issues which will allow you to rename the generated names of schema types, including custom scalars. This will help you get around the naming conflicts with things like Foundation.Date.

Mordil commented 9 months ago

That doesn't necessarily help us with conflicts between ApolloAPI.Object and Realm.Object, however.

I understand the rationale for the default as it is, because it does simplify using things as you described by not requiring import ApolloAPI - however, this is no less burdensome than those of us in this situation who now have to full-specify all of our types from these conflicts anyway, and arguably makes it worse.

AnthonyMDev commented 9 months ago

Yeah, that definitely doesn't feel great either right now. FWIW, there is some pretty extensive discussion of this topic in this issue already. But I don't think ApolloAPI.Object specifically existed at that time.

I think the parts of ApolloAPI that actually are better off exported are pretty small. GraphQLNullable and GraphQLEnum definitely should be, but I don't think ApolloAPI.Object should be exported anyways. So I think the way to handle this is to have an SPI that only contains the few things that really should be exported as well. We should ensure that anything in that SPI has names that are not too generalized and are unlikely to have lots of conflicts. GraphQLNullable can be exported pretty safely for 99% of people, but not Object.

AnthonyMDev commented 9 months ago

Basically we would have:

Then the generated models end up having their import statements look like:

@_exported @spi(exports) import ApolloAPI
@spi(internal) import ApolloAPI

Though I'm concerned that the compiler won't let that work, due to seeing it as duplicate import statements for ApolloAPI. In which case, maybe the exported things need to be in their own new module ApolloAPIExports or something like that.

BobaFetters commented 9 months ago

@AnthonyMDev Wouldn't we want to have everything that would be useful to consumers still just be regular ApolloAPI with the @_exported import ApolloAPI as is, and then take everything that doesn't need to be exported by default or is internal and move those into @_spi's so that users can import them at will?

AnthonyMDev commented 9 months ago

There are a lot of parts of ApolloAPI that are useful for that parts of your app code that configure your networking and cache layer. Custom interceptors, cache key configuration, and custom cache mutations for example. Those parts should be public because there is use case for them and they are intended for public consumption. However they don't need to be exported to all of your other code that just consumes the generated models.

By exporting all of ApolloAPI, these components are exported everywhere and you get issues like the conflict between ApolloAPI.Object and Realm.Object. If those issues were isolated to only files that needed to work with them and explicitly import ApolloAPI, the namespacing becomes a lot less cumbersome. And for all the rest of your codebase, you don't get them coming up in code completion when they aren't appropriate.

There are also pieces that are not intended for public consumption, but are consumed by the generated models. These pieces should be part of the @_spi(internal).

The only pieces that need to be exported are the pieces that you need to use to interact with the generated models, like GraphQLEnum and GraphQLNullable. Which is why I'm recommending 2 different SPIs, ultimately giving us 3 different access levels that can be used for each of these use cases.

BobaFetters commented 9 months ago

Yea I was just recommending that maybe for things like GraphQLEnum and GraphQLNullable they remain as is with no spi so they can still be used through the existing @_exported import ApolloAPI` and then everything else can be broken down into however many spi's we feel are necessary to provide targeted imports users can call if needed.

github-actions[bot] commented 8 months ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.