-
Notifications
You must be signed in to change notification settings - Fork 734
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
Apollo 1.0 Migration Feedback #2678
Comments
Hi @CraigSiemens 👋
Let's see if we can ease some of those concerns and get you back on track to adopting
This doesn't sound like a particularly unique configuration and I would be surprised if there isn't an Apollo codegen configuration that doesn't work for your project.
You should still be able to do this although no longer with just a single output file. Unless there is any specific logic in your swift package for the code generation you should be able to achieve the same with the CLI + configuration file.
There is additional complication because
I agree, and we have #2642 to address this.
They should be thought of as separate 'groups' of output, which is why they each have a configuration option.
This request is coming from a few places now and we've got #2630 to track it's progress. In hindsight it's something we should have carried into 1.0 initially.
This is specific to CocoaPods because of the way it aggregates and exposes the imported dependent Apollo modules by the pod name. Access to
We assume responsibility for using the underscored attributes and will adapt to any changes in their availability. We're aware of the potential risks and are comfortable with them.
I'll come back to you on this one. |
At first pass this sounds like a combination of This keeps the schema types in their own module that can be shared around, with the operation files relative to the modules where they're used. Unfortunately they can't be completely locked down right now because of the lack of an access modifier setting. |
I have also attempted to migrate our app to Apollo 1.0 in the last week before the holidays. I anticipated a lot of breaking changes going into it but I thought it'd likely be a relatively straightforward process. I was wrong. I apologise in advance if any of this comes across as overly emotive - I know everyone works extremely hard and are trying to do the best I can. But I also suspect there will be many others attempting this migration who will have more than a few under the breath grumbles as they do it, and if my grumbles can be alleviated it's going to make it a heck of a lot easier for someone else to come in later. This migration is by far the most complicated and involved migration I've done since the 'we changed everything!' release of Swift 3.0. But at least that had fixits! Our app is a lot smaller than OP's 90 module app - we essentially have just one API module which does a similar thing of mapping GraphQL types to our own types to keep things separate. Arguably it's a pretty simple module, but seeing how difficult a time I've had this week really makes me suspect you're going to end up with a lot of teams sticking with 0.53 for a long, long time. The fundamental problemFundamentally I believe the major problem with this Apollo release is that it does too much. It shouldn't be 1.0, it should be something like 5.0, with each of the many, many breaking changes spread out across releases so we can migrate one part at a time rather than Big Bang it and have no confidence that I'm not breaking anything along the way. I can't test one part of it until I've done the whole thing because of the innumerable compiler issues that need to be fixed, so I can't verify anything in isolation. Setup issues and getting the generated code to compileRight off the bat, your migration guide suggests installing a new CLI tool (which, installing from Xcode didn't even work until the 1.0.5 release - it's the first step!), then suggests setting up an entirely new Swift package for the generated files. Yes, you can just pick a directory, but then you lose the namespacing, so you really want to go the package route. Frankly, an architecture change as major as this should be step 10, not step 1, when I just want to get things compiling again. Once I had my new Swift package, it didn't even compile! So, off to GitHub Issues I go, and then I find this issue. So, then I had to rename all my fragments to add the 'Fragment' suffix to avoid name collisions. We have lot of fragments, so that's going to cause a lot of code changes. Next step was setting up scalars again, which have a similar, but slightly different API and need to live in the new Swift package now. This is where I was starting to think this release needs to be broken up across several major releases, though to be fair this step was at least relatively straightforward once I worked out what I needed to do. Though I did have to do a couple of silly typealiases like Finally, the module compiles, so it's time to fix my actual code. 99 compiler errors to go, recompile, 109 compiler errors to goFirst up, can I just say how amazingly thankful I and the rest of my team are that fragments are generated in a much better way now (much the same as the Android SDK) - previously we had a lot of problems mapping them to our own types, and this seems to be massively cleaned up, and I was able to delete a lot of boilerplate glue code we had. Then came the need to put Eventually I breathed a sigh of relief that I was able to get the thing to compile. Time to fix the tests! TestsThis is where I nearly threw my laptop out the window. To test all our mappings, we have hundreds of tests which directly instantiate the I'll take the migration guide's assertion that this was 'cumbersome, error-prone and fragile' at face value (it worked well enough for my use case, anyway - it's typesafe and easy to do). But I'd humbly suggest that instead of just nuking the old way from orbit that you keep it in and add in deprecation warnings? Apollo 1.0 is already massive enough. That way I can at least migrate across slowly rather than now having to fix hundreds of files in one go. Other smaller issues
tldr; I totally get that a major release will have, well, major changes. But that many? I honestly want to pause migration as well but am dreading having to go through this pain again in future so I'm trudging through. At the time of writing I've only fixed one test file, after my 100th I might be of a different view. |
The Install CLI tool was introduced in 1.0.4 to address the problems users' were experiencing in Xcode with the previous SPM plugins. That history is all in the changelog.
I'm not sure what you mean by "lose the namespacing". If you can share some more detail I'd like to understand the problem better. When using the
This has been a common piece of feedback and something we'll be addressing within the next couple releases.
If you have suggestions, or a PR, for improvements to the Custom scalars portion of the migration guide we'd love to see it.
Another option is to build and maintain your own extension for the struct initializer, it may have been easier than modifying all the tests. Plus has the benefit of being able to be used for SwiftUI previews, if your project uses them. No, having to create the initializers yourself is not an optimal solution. This is another contentious change that we may have to review and workaround.
What is the setting you're using for test mocks in the codegen configuration?
Unfortunately not.
There is a property
In an ideal world Apollo iOS would have a far better version history of getting to a first major release and beyond. Which would lead to the more evolutionary progression you're after. But it doesn't and we're now on the path of overhauling the three major parts to the SDK in each of the next major versions.
Code generation I think will be most disruptive because of the major ways in which the models changed. Networking might be too since the interceptors paradigm is not great. Caching much less because its feature minimal at the moment so 3.0 will mostly be additive. No, these changes are not easy depending on the complexity of your project. We appreciate the feedback and will take these learnings into the next major releases. We strongly believe that the changes now are building a better foundation on which we can build an even better product. |
Thanks for all your feedback! @CraigSiemens
This sounds really awesome! I'd love to get my hands on the infrastructure you've built here and see if we could eventually integrate it into a tool to provide to others.
I think the generated group of "schema" files should be included in this module, it generates template files for your scalar types and all of the necessary files that your generated operations need.
Yeah, we are aware of this limitation. I know it's not ideal, but Swift can't deduce switch exhaustivity with the pattern matching operator. We provide it as a convenience, but to do what you are asking, there are a few options. If you need to be able to handle the unknown cases using their let enum: GraphQLEnum<MyEnum>
switch enum {
case let .case(value):
switch value {
case .myValueA: // ...
case .myValueB: //...
}
case let .unknown(rawValue):
// handle rawValue
} If you just have a fallback when the value is unknown that doesn't care about the let enum: GraphQLEnum<MyEnum>
switch enum.value {
case .myValueA: // ...
case .myValueB: //...
case .none:
// handle nil case for unknown values
} Either of these will give you exhaustivity and not require a |
And thank you as well @jarrodrobins I've got some things to add. Overall, I do understand your frustration, the 1.0 is a huge release with a lot of fundamental breaking changes. We really re-thought a lot of the underlying principles to get to a point where we had a stable API that we felt we could build on in the future without having to make major breaking changes continually. We decided to go the route of ripping the band-aid off all at once. While that is going to make the initial migration a lot more work, we felt like it also allowed us to have a much higher quality product. A lot of these little things were tied to each other in ways that are not immediately obvious, and trying to incrementally do a few things at a time, we would have had to make a lot of sacrifices that we felt would have deteriorated the quality of the end product. It also would have probably caused a lot more bugs over time as we handled the edge cases of all the incremental improvements being backwards compatible with the stuff we hadn't fixed (broken?) yet.
Because initializing test mocks with straight JSON data is error-prone, we created the TestMocks framework, but you can still do what you were doing before, the initializer is just a bit more cumbersome.
This seems like a reasonable request! Please file an issue and we'll add it to an upcoming minor release!\
As Calvin stated, |
Thanks both @calvincestari and @AnthonyMDev for the replies. I totally understand why you've gone for the big rip-off-the-bandaid release, as rewriting any piece of software will generally necessitate that in some respects, and I do acknowledge making all the changes will end up with the better product even if the initial migration has its share of pain.
That's my mistake - I initially tried upgrading to 1.0.0 rather than 1.0.5 to minimise changes, but the docs had also changed. That's on me. D'oh.
Despite my typing and pre-holiday brain mush I was actually referring to access modifiers and how all the generated code is public as opposed to internal. Looks like that's planned on being fixed in #2630.
I ended up getting the test mocks to compile - think this may have been Xcode playing up. It'd have problems finding ApolloAPI for some reason. Don't ask me what I changed - removing and readding the framework to my test target seemed to work. 🙈
We might be talking about different things here as I wasn't going anywhere near SelectionSet - that would be very error prone, I agree! What I was doing was this -
whereas now, I need to do this -
The additional burden here (and which I've spent the last few days on) is that the mocks also have a different ordering for all the parameters! They're now alphabetical as opposed to the order they were put in in the GraphQL file. Xcode is fairly unhelpful with fixits so this was a lot of manual stuffing around putting everything in the correct order. In future it may be helpful to have some sort of configuration option here.
Will definitely do this, thanks.
You can grab it from a concrete type, but not from the generic If you're interested in any metrics, I have just finished our migration and this is the result of my PR with the vast majority of the changes being in my test files. Definitely not a small one! |
For your test mocks, is all the qualified name spacing required to make your code compile? That's definitely verbose and ugly. Not my intentions. My hope is that this can be reduced down to at least this: let response = Generated.MyQuery.Data.from(
Mock<Query>(
getCards: Mock<PaginatedCards>(
cards: [
Mock<Card>(
activationStatus: .case(.activated),
cardType: .case(.physical),
createdDate: Date(),
id: id,
nameOnCard: "xxx",
nickname: "yyy",
...
)
],
hasMore: false
)
)
)
Can you use
I'd love more insight into why it's such a large addition of lines of code! I'm hearing that your test mocks are a lot more work now, but one of the main goals was to decrease the size of the generated code significantly, so I would assume to see a migration resulting in less lines in most cases. |
I'm going to close this, just because it's not an concrete action item for us to work on. We are happy to continue the discussion and receive more feedback on this issue. Please feel free to keep commenting! It just doesn't need to be in our open "issues" list. We're not going to disallow discussions on GitHub issues, but for things that are discussions/feedback, rather than individual actionable feature requests/bugs, it might be more appropriate to use the Apollo Discourse Boards instead of GitHub issues in the future! |
We still haven't had any more time to devote to this but hopefully some of the suggestions will help.
Here's the really high level version of it.
|
@CraigSiemens
The new codegen engine was written in Swift to make it easy for you to alter the code generation templates! Rather than calling into a separate command that alters the written files, it would be really, really easy for you to fork the codegen engine and modify the templates to include these changes! All 3 things that you have on that list are features that we do aim to make available via configuration in the future. So if you ever consider make any of these changes in the code gen engine itself, adding them as configuration options would make it feasible for us to accept a PR for these into the main repo! |
We finally got around to finishing this migration in our project. The final stats were:
Many of the schema types are duplicated across modules so there's still some work that can be done to reduce the total amount of code. |
We've been spending a couple days trying to update our project to use Apollo 1.0 but unfortunately it hasn't really been smooth sailing. For now we're probably going to pause our migration to 1.0 because the benefits don't yet outweigh the complexity in upgrading.
Also, sorry for the wall of text. I hope it's better than creating multiple issues for now since there's some overlap between the suggestions.
Our Project Structure
We have ~90 modules in our project with ~50 of them containing
.graphql
files requiring codegen. Most of those modules represent a self contained feature of the application with UI, models, and API code it needs. We try to keep the Apollo types isolated to the API layer and map them to our own model types before passing them onto the UI or other modules.Each of those modules has a build phase to run the codegen when any of the graphql files are changed (using input xcfilelists). We have a swift package that is using
ApolloCodegenLib
. It runs codegen generating the single file in a temporary directory, makes some changes to the file, then moves it to the location in the project only if it's content is different so Xcode doesn't think it has to rebuild the file.We also have a base API module that creates the interceptors,
ApolloClient
, andScalar
type definitions for all the modules to use.Issues and Suggestions
The new codegen seems to be trying to help out with a projects module structure, but it feels like it makes the codegen options more complicated and less flexible than it could be. I’ve been trying to infer the supported structures from various options but it’s pretty complicated cause it feels like there’s hidden interdependencies between some of them.
For our project we’d just need a way to set the
That would still allow having convenience inits for the options built on top of those while still allowing customization when needed.
schemaName
would be clearer if it was callednamespace
, which was the old name in the codegen. That seems to be what it’s mostly used for. The current name makes it seem like it’s related toschemaFile
andschemaTypes
when it isn’t exactly referring to the same things. I think it's also sometimes used as as the name of a module and imported in operations.If it's expected the schema types and operations can be written to separate locations, it'd probably be simpler if each could customize the namespace each uses and, with the suggestion above, directly control what import statements are added.
We’d like to limit the exposure of graphql types within our project so it’d be nice if there was an option to control the ACL rather than everything being public. The old codegen had an option for it but I believe it only worked with the experimental swift codegen, which I guess the new one replaced.
We previously had our script do a find and replace of public to internal. This was easy enough when only one file was generated, but now that there’s multiple it be more complicated.
cocoapodsCompatibleImportStatements
could have a name that isn’t dependent on a specific decency manager. Even though we don’t use cocoapods, I feel like we may have to use this option for other reasons.With the suggestion above adding better control over import statements, this could potentially be replaced by a convenience init, or a constant that can be passed for the imports to use.
It'd be nice to have an option to remove the
@_exported
on the imports. Again with our module structure we don’t want to be exposing Apollo to places that don’t need it. We also try to avoid using anything prefixed with underscores if we don’t have to.Having control over the imports added would also solve this.
The custom pattern matching on
GraphQLEnum
can’t be used while also making a switch statement exhaustive. Adefault
case is always required so the compiler can’t automatically detect when a new case is added. That makes it very easy for a developer to miss that a new case was added and be aware that the app should add handling for the new case.While the old codegen creating
__unknown
always looked a little weird, it allowed a simple way to create exhaustive switch statements with the case names directly. It also allowed Xcode to autocomplete all the switch statement with all cases or add missing cases. Having to addcase .case(.someEnumCase)
for every case to get an exhaustive switch isn't a great experience.The text was updated successfully, but these errors were encountered: