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

Delete unused imports and declarations #3100

Merged
merged 13 commits into from
Jul 20, 2023

Conversation

Iron-Ham
Copy link
Contributor

@Iron-Ham Iron-Ham commented Jul 3, 2023

Closes #3099

@netlify
Copy link

netlify bot commented Jul 3, 2023

👷 Deploy request for apollo-ios-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit eee71b7

@Iron-Ham Iron-Ham changed the title Delete unused imports Delete unused imports and declarations Jul 3, 2023
@AnthonyMDev
Copy link
Contributor

@calvincestari have you ever seen this Circle CI error before?

@calvincestari
Copy link
Member

The OAuth error? I've only seen that one when Apollo changed security config and we had to remove/add our auths. Should have been resolved a while ago. Try running it again.

@Iron-Ham
Copy link
Contributor Author

Iron-Ham commented Jul 7, 2023

Don't think that's something I can do on my end

@calvincestari
Copy link
Member

I'm limited to mobile till the weekend but I don't think this is anything wrong in your PR.

@calvincestari
Copy link
Member

I've followed up with our security team to see if we can figure out what is going on, it appears limited to forked repo contributions so let's see what they come back with. I'll update you once I learn more.

@calvincestari
Copy link
Member

OK @Iron-Ham, it looks like it got a bit further with that empty commit I pushed. I've been told that you should try fully logging out of GitHub to see if that helps? In the past I followed these steps for CircleCI but you wouldn't have those credentials so I'm hoping the GH logout/login will resolve it for you.

@Iron-Ham
Copy link
Contributor Author

@calvincestari hopefully that does it? Double-checked if I had some old CircleCI tokens on my account from previous jobs but no dice.

@calvincestari
Copy link
Member

It looks like your fork might be quite far behind the upstream. I'm guessing that's the reason for the failed tests?

This branch is 4 commits ahead of, 45 commits behind apollographql/apollo-ios:main.

@Iron-Ham
Copy link
Contributor Author

@calvincestari merged with main -- could you try an empty commit again?

Copy link
Member

@calvincestari calvincestari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup PR @Iron-Ham. The import removal looks good, I'm less certain about the RawData type changes though.

@AnthonyMDev it looks like the opaqueObjectDataWrapper should have been using the RawData type alias to create the consistency we were looking for - can you confirm that please. I can't find any other reference to them so we either use that type in the function declaration or remove the type alias.

@Iron-Ham
Copy link
Contributor Author

@calvincestari the RawData aliases were fully unused outside of their declaration – and I was torn between either removing them fully, or inferring that it should have been used in the files that they were declared in.

@calvincestari
Copy link
Member

or inferring that it should have been used in the files that they were declared in.

I think this is the right choice. My opinion is to use them because it creates the consistency when reading the code that we wanted to achieve with that change, even if they have different types under the hood.

@AnthonyMDev
Copy link
Contributor

I've been hesitant to remove Foundation imports because I've had situations in the past where code compiled in Xcode for our project after removing them, but then when actually included in someone else's project as a package (I think w/CocoaPods?) they would fail to compile.

I think this has to do with umbrella headers or something where Foundation was always being linked against targets due to our project configuration, but that wasn't the case when consumed as a package.

If the files really don't use Foundation then this is fine, which I'm assuming is the case because the static analyzer only identified some specific import statements to be removed. So I'm okay with this change.


The RawData was supposed to be a typealias that explictly declares the type for the GraphQLExecutionSource protocol's associated type. At some point, I changed the associated type name from RawData to RawObjectData. Now the compiler is having to infer the associated type from the function signatures.

I think the proper fix would be to change the typealiases to RawObjectData, but I think it's still easier to read and reason about the code by leaving the references in the actual function signatures and implementations as the direct type. The point of the typealiases in the first place was just to give the compiler the explicit information it needed to shave nanoseconds off of compilation time. 😅

@calvincestari
Copy link
Member

I think the proper fix would be to change the typealiases to RawObjectData, but I think it's still easier to read and reason about the code by leaving the references in the actual function signatures and implementations as the direct type. The point of the typealiases in the first place was just to give the compiler the explicit information it needed to shave nanoseconds off of compilation time. 😅

Good to know, thanks for that context. It appears that RawObjectData is correctly used in CacheDataExecutionSource already so I'm going to update the two other sources and then I think we can get this merged.

@calvincestari calvincestari merged commit b2a3054 into apollographql:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static Analysis Results: Unused Code
3 participants