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

[Swift] Fix build failures for optionals #1462

Closed
wants to merge 4 commits into from

Conversation

@designatednerd
Copy link
Contributor

commented Aug 6, 2019

This PR fixes issues introduced by #656, which I prematurely had released. 🤦‍♀️

The issues are most obvious in this failed run of the main iOS SDK, but the short version is that we weren't fully checking for optionals with that code, and the compiler was pretty angry about it.

designatednerd added some commits Aug 6, 2019

Remove unnecessary symbol
love it when npm refuses to run tests locally

@designatednerd designatednerd requested a review from martijnwalraven Aug 6, 2019

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Gonna get a look from @martijnwalraven later today, I know he had a couple concerns about it clobbering new conformances to a type on the server

);
this.withinBlock(
() =>
this.printOnNewline(
`${structName}.possibleTypes.contains($0.__typename)`
`guard let type = $0 else { return false }; return ${structName}.possibleTypes.contains(type.__typename)`

This comment has been minimized.

Copy link
@koenpunt

koenpunt Aug 6, 2019

Contributor

For readability it might be nice to have this on 2 lines instead.

guard let type = $0 else { return false };
return ${structName}.possibleTypes.contains(type.__typename)

This comment has been minimized.

Copy link
@koenpunt

koenpunt Aug 6, 2019

Contributor

Also, I'm trying to see how $0 can be nil after being flatMapped.. That said, we might want to replace flatMap with compactMap since flatMap is deprecated.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Aug 6, 2019

Author Contributor

For some reason this was being real dumb about letting me use more than one line, and I really didn't feel like fighting with typescript at that moment.

And I suspect it's because we're using flatMap where we should be using compactMap, but I'd like to hear some thoughts from @martijnwalraven before I rip this apart any further.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Aug 6, 2019

Author Contributor

(OK i at least got this onto a separate line)

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@koenpunt @martijnwalraven I took a stab at getting the swift code to look a bit simpler just through turning off codegen and refactoring:

Screen Shot 2019-08-06 at 1 54 23 PM

I think this should prevent crashes since Friend just gets the unsafe result map with the unknown type and you don't need to worry about the type in that instance.

The problem is that the existing knot of flatMapping is all done in mapExpressionForType, which from the looks of the callers is doing a WHOLE bunch of other stuff I don't totally understand.

Any suggestions on that would be most appreciated.

@koenpunt

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@koenpunt @martijnwalraven I took a stab at getting the swift code to look a bit simpler just through turning off codegen and refactoring:

That looks way more readable! But indeed mapExpressionForType covers a lot of things. Maybe if @martijnwalraven can shed some light on what all these methods (typeNameFromGraphQLType, mapExpressionForType, etc) do, that could really help. But given the fact it's about 2 years that code was written, it might even be hard for him to recall how everything is working.

@koenpunt

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

I think this should prevent crashes since Friend just gets the unsafe result map with the unknown type and you don't need to worry about the type in that instance.

This doesn't crash when the value in the array is optional (gql; friends: [Friend]), but for required types (gql; friends: [Friend!]!) a compactMap would still be required I think.

@martijnwalraven

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@koenpunt Can you help me understand what issue you’re trying to solve? Where does the crash when adding a new implementing type for an interface come from?

There is probably a bug in the current generated code. But I don't think filtering out objects of unknown types is the answer. Introducing a new object type should be backwards compatible if you’re accessing it through an interface. Let’s say you have:

interface ContentItem {
  title: String
}

type BlogPost implements ContentItem {
  title: String
}

type Query {
  favoriteItems: [ContentItem]
}
query {
  favoriteItems {
    title
  }
}

If the server then introduces:

type Poem implements ContentItem {
  title: String
}

and adds a new item of type Poem to the list of favoriteItems, it should still be returned from the query and be accessible as a ContentItem (so you could get its title like you would for any other implementation of that interface).

As you mentioned, it's been a while since I worked on this codebase, so it's very likely I made a mistake. But if you agree the above makes sense as the behavior we want, we can figure out what changes we need to make to the generated code.

@martijnwalraven

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@koenpunt @martijnwalraven I took a stab at getting the swift code to look a bit simpler just through turning off codegen and refactoring:

That looks way more readable! But indeed mapExpressionForType covers a lot of things. Maybe if @martijnwalraven can shed some light on what all these methods (typeNameFromGraphQLType, mapExpressionForType, etc) do, that could really help. But given the fact it's about 2 years that code was written, it might even be hard for him to recall how everything is working.

One of the reasons mapExpressionForType is complicated is because it has to deal with nested types, like a non-null list of nullable lists of non-null elements (e.g. [[BlogPost!]]!). We may be able to simplify the generated code, but having it output expressions instead of intermediate variables makes it a lot easier to deal with nesting because we can just call mapExpressionForType recursively.

@koenpunt

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Where does the crash when adding a new implementing type for an interface come from?

The crash is not with interfaces (or at least not what I experienced), but with union types.

So when a schema like this:

union ContentItem = BlogPost

type BlogPost {
  title: String
}

type Query {
  favoriteItems: [ContentItem]
}

Gets updated to;

union ContentItem = BlogPost | Image

type Image {
  url: String!
}

type BlogPost {
  title: String
}

type Query {
  favoriteItems: [ContentItem]
}
@martijnwalraven

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@koenpunt There isn't really a difference between interfaces and unions in terms of the generated code. And they are in fact very similar in GraphQL, the main difference is where the implementing types are declared. (Another difference is that unions don't define fields themselves, so the returned unknown object type would be pretty useless unless it also implements a known interface.)

I'm wondering what specifically is causing the crash. Which line in the generated code is responsible?

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@koenpunt Would really appreciate it if you could let us know - I may wind up needing to temporarily revert #656 on Master if we can't get this figured out in the next day or two as it's blocking a release on the iOS SDK.

@koenpunt

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I'm wondering what specifically is causing the crash. Which line in the generated code is responsible?

Can't recall, it's been 9 months :) and the schema changed in the meantime, so the problem currently doesn't exist in our project. I will create a reproduction project, but for now you can revert, since no one else reported this issue (I believe).

@designatednerd

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

OK @koenpunt I'm going to close this one out - can you please open a new issue and tag me when you're able to repro? Thank you!

@koenpunt

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@designatednerd I'm not able to reproduce it atm, so it might be that it has been fixed (accidentally) with some recent refactor. Or I'm not recreating the same scenario I had in the past, but that is something I can't know for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.