Deprecated types are not marked as deprecated in code gen

[dfed]

Summary

While deprecated properties are marked as deprecated in generated code, deprecated types are not

Version

1.15.3

Steps to reproduce the behavior

1. Create a schema file with:

type Subscription {
  campaignTaskActive(taskID: String!): CampaignTaskEvent!
  campaignTaskActivate(taskID: String!): CampaignTaskEvent! @deprecated(reason: "Use `campaignTaskActive` instead")
}

type CampaignTaskEvent {}

2. Create a .graphql file with:

subscription CampaignTaskActivateSubscription($taskID: String!) {
  campaignTaskActivate(taskID: $taskID) {
    __typename
  }
}

3. Generate the graphql code using apollo-ios-cli generate.

Expected results:
Referencing the Swift type CampaignTaskActivateSubscription would cause a deprecation warning because the CampaignTaskActivateSubscription is deprecated.

Actual results:
The generated Swift type CampaignTaskActivateSubscription is not deprecated.

Logs

No response

Anything else?

No response

[calvincestari]

I think the current behaviour is correct because it doesn't look like the @deprecated directive can be used on a type; it's limited to FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE

Hmm, maybe we have a bug on the root types?

[aseemk]

In @dfed's example schema above, campaignTaskActivate (@deprecated) is a field on type Subscription.

[aseemk]

And if helpful, this is valid GraphQL per graphql-js via apollo-server. We deprecate mutations and subscriptions like this all the time, and they correctly show up as deprecated in Apollo's web UI.

[calvincestari]

I think this might be by design, I'll have to check with @AnthonyMDev. 😕

Codegen is correctly adding the deprecated attribute to the field within the enclosing Swift class's Data property. This behaviour is the same across queries too.

[calvincestari]

Like I said, this might be a bug on the root types. I can understand the expectation that the deprecation attribute is placed on the enclosing Swift class too but right now I'm not sure.

[calvincestari]

@AnthonyMDev and I had a long chat about this one today and we came to the conclusion that the current behaviour is correct.

What would be your expectation if your subscription were written as this?

subscription CampaignTaskActivateSubscription($taskID: String!) {
  campaignTaskActivate(taskID: $taskID) {
    __typename
  }
  campaignTaskActive(taskID: $taskID) {
    __typename
  }
}

With the current behaviour it's only the campaignTaskActivate property that would be marked as deprecated and you could still correctly access campaignTaskActive. This seems like a very practical way to go about moving off of a deprecated field where you might encounter 1 out of x call sites that is difficult to work through and you need more time to resolve it while the rest of your code can make use of the new field.

Another point to consider is that the schema should not be able to deprecate the subscription operation since those are defined external to the schema (by clients typically). It can deprecate a subscription field, as you've shown above, which correctly results in the subscription field being marked as deprecated in the subscription operation.

@dfed / @aseemk - I'm keen to hear your thoughts on the above.

[dfed]

TIL that it was possible to put multiple subscriptions in the same object!

That said, my high level goal is: when a type or field is deprecated, I want my code-gen to alert me that I am using something that is deprecated.

In the above case, I think it would be reasonable to mark the enclosed object as deprecated because it is using a subscription that is deprecated.

Often I'm not accessing the fields on the subscription, and instead relying on Apollo's store to notify watchers of field changes. So with the current code-gen there's no warning that I'm using anything deprecated.

Note that deprecations are warnings: it's reasonable to have warnings in a codebase until the codebase has stopped using deprecated fields.

[calvincestari]

In the above case, I think it would be reasonable to mark the enclosed object as deprecated because it is using a subscription that is deprecated.

Even if you only ever used it for CampaignTaskActivateSubscription.campaignTaskActive? campaignTaskActivate being marked as deprecated in the schema is something the client has no control over and wouldn't be using. Therefore marking the entire CampaignTaskActivateSubscription as deprecated seems over-reaching.

Isn't this akin to:

enum CampaignTaskActivateSubscription {
   @available(*, deprecated)
   case campaignTaskActivate
   case campaignTaskActive
}

Would you expect a deprecation warning if you used it as:

if case let CampaignTaskActivateSubscription.campaignTaskActive = value { ... }

Often I'm not accessing the fields on the subscription, and instead relying on Apollo's store to notify watchers of field changes. So with the current code-gen there's no warning that I'm using anything deprecated.

I think this is what forms the basis of your expectation to which I'd say we need to find another solution.

[dfed]

Even if you only ever used it for CampaignTaskActivateSubscription.campaignTaskActive?

It's very possible I'm missing something, but is it possible to just use CampaignTaskActivateSubscription.campaignTaskActive in the above example? If I'm subscribing to the CampaignTaskActivateSubscription subscription, then both campaignTaskActivate and campaignTaskActive are being sent to the server, right? Is there a way to utilize the CampaignTaskActivateSubscription object without also utilizing the deprecated campaignTaskActivate?

If the answer is "no", then I'm struggling to see the parallel to the enum example. If the answer is "yes", then I need more info before I can understand the implications of my proposal above.

I think this is what forms the basis of your expectation to which I'd say we need to find another solution.

I'm certainly open to the idea that my approach is unique or otherwise in the minority. But I am hopeful that we're aligned on a goal of "if the server marks an operation as deprecated, client-side code-gen that utilizes the deprecated operation should warn".

Where we might be disconnected is whether the thing that is deprecated is the returned value or the operation itself. From my and @aseemk's POV, the operation is the thing being deprecated.

[calvincestari]

Even if you only ever used it for CampaignTaskActivateSubscription.campaignTaskActive?

It's very possible I'm missing something, but is it possible to just use CampaignTaskActivateSubscription.campaignTaskActive in the above example? If I'm subscribing to the CampaignTaskActivateSubscription subscription, then both campaignTaskActivate and campaignTaskActive are being sent to the server, right? Is there a way to utilize the CampaignTaskActivateSubscription object without also utilizing the deprecated campaignTaskActivate?

We're both correct, because we're meaning different things.

• You are right that both fields will get sent to the server for execution, yes.
• When I said "only ever used it for" I mean accessing the non-deprecated property on the generated Swift model.

So my analogy to the enum is that when accessing the non-deprecated enum case CampaignTaskActivateSubscription.campaignTaskActive directly I would not expect to get a warning. The same as I would not expect to get a warning when accessing the non-deprecated field in the operation.

Where we might be disconnected is whether the thing that is deprecated is the returned value or the operation itself. From my and @aseemk's POV, the operation is the thing being deprecated.

This is the crux of our differences. A field does not equal an operation, they are not 1:1. campaignTaskActivate is simply a field on type Subscription. One of possibly many. There are no operations defined in the schema.

I'm certainly open to the idea that my approach is unique or otherwise in the minority. But I am hopeful that we're aligned on a goal of "if the server marks an operation as deprecated, client-side code-gen that utilizes the deprecated operation should warn".

With the same caveat that an operation cannot be deprecated, Apollo iOS does already do this. You will correctly get a warning when accessing the deprecated Swift property matching the field that is annotated as deprecated in the schema.

We're not going to be making any changes to the generated Swift code to mark entire operations as deprecated. I think the next best thing we can begin to look at is a warning in the code generation output log but this is not intuitive, nor will it be raised in Xcode so it's of lesser value.

[dfed]

A field does not equal an operation, they are not 1:1.

Got it. This is a good distinction, and helps me see your POV here.

With this information, I think I need to update my goal to: "There should be an automated a way for a client to know it is generating code that the schema has marked as deprecated"

I think the next best thing we can begin to look at is a warning in the code generation output log but this is not intuitive, nor will it be raised in Xcode so it's of lesser value.

If I could run CLI code-gen with a flag that will exit(1) with a good error message when we're generating code for a deprecated field, that would absolutely solve my problem. I just want a way to have high confidence that my iOS client has entirely migrated off of deprecated fields.

[calvincestari]

If I could run CLI code-gen with a flag that will exit(1) with a good error message when we're generating code for a deprecated field, that would absolutely solve my problem. I just want a way to have high confidence that my iOS client has entirely migrated off of deprecated fields.

Either a command line option or a codegen config option that would communicate fail-when-deprecated-field-would-be-sent-to-the-server-for-execution (aka. used in an operation). I'll have to see how the rest of the team feels about this though.

As for timing of when the error could be raised..I originally thought this would have to happen quite late in code generation because it's not violating any GraphQL syntax or validation rule. It's only when we get to generating the Swift code that we would know if @available(*, deprecated) is needed. This might be once we've written X of Y Swift files already though.

Giving it a second thought, it might be able to happen directly after document (schema and operation) validation. Codegen only pulls the schema types that are used in operations so we could iterate through all the pulled types looking for those with deprecated fields and exit(1) at the first match.

[AnthonyMDev]

Thanks @calvincestari & @dfed for the great discourse one this. After reading all of the discussion. I still don't think this is something we should be solving for. This is not a widely requested feature, and we try to restrain from adding more configuration options to the codegen for use cases that aren't widely applicable. The config is already large and complex and we need to be judicious about its expansion.

But I had another idea for you last night @dfed. What you are asking for can be achieved using SwiftLint, or any other linter tool with a custom rule. Just set it up so you get a linter warning whenever a file that ends in .graphql.swift includes the string @available(*, deprecated, message:.

[dfed]

What you are asking for can be achieved using SwiftLint, or any other linter tool with a custom rule. Just set it up so you get a linter warning whenever a file that ends in .graphql.swift includes the string @available(*, deprecated, message:.

Smart!! Yes that works, and solves my problem.

Very much appreciate the discussion. I understand the motivation to lower long-term maintenance burden by not supporting less-common requests. You've led me to the place I need to solve my own problem, which is what I really needed. Closing this Issue out!

[github-actions]

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.