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

Codegen Option: Optional Exported Imports #3322

Closed
Mordil opened this issue Jan 18, 2024 · 8 comments
Closed

Codegen Option: Optional Exported Imports #3322

Mordil opened this issue Jan 18, 2024 · 8 comments
Assignees
Labels
feature New addition or enhancement to existing solutions

Comments

@Mordil
Copy link
Contributor

Mordil commented Jan 18, 2024

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

@Mordil Mordil added the feature New addition or enhancement to existing solutions label Jan 18, 2024
@AnthonyMDev
Copy link
Contributor

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
Copy link
Contributor Author

Mordil commented Jan 22, 2024

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
Copy link
Contributor

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
Copy link
Contributor

AnthonyMDev commented Jan 22, 2024

Basically we would have:

  • @_spi(exports)
    • includes things that should be exported
  • Just regular public,
    -entities like Object, that may be consumed by users in parts of their infrastructure that directly import ApolloAPI, but aren't necessary just for consuming the generated models.
  • @_spi(internal)
    • Things that need to be used by the generated models internally, but should not be used by consumers of Apollo directly.

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
Copy link
Member

BobaFetters commented Jan 23, 2024

@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
Copy link
Contributor

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
Copy link
Member

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.

@AnthonyMDev AnthonyMDev closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2024
Copy link

github-actions bot commented Mar 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Development

No branches or pull requests

3 participants