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

fix: Defer metadata extensions #581

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

calvincestari
Copy link
Member

Fixes apollographql/apollo-ios#3503

This implements solution 2 in the issue description for the following reasons:

  • This diff is minimal and resolves the issue for all module types. We could keep the extension but we're going to end up with multiple templates that need to handle the metadata insertion = IMO less optimal.
  • I don't see any benefit to having the defer metadata within the same file but in an extension. The metadata is partitioned with a MARK comment line.
  • Operation files also do not support custom edits so there is no need to preserve anything about previously generated operation files.

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Jan 21, 2025

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: ef6b2e08b931f268a3cf7ce0

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for apollo-ios-docc canceled.

Name Link
🔨 Latest commit 3180ac0
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docc/deploys/678ef1b3caf5ee000802dc1d

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for eclectic-pie-88a2ba ready!

Name Link
🔨 Latest commit 3180ac0
🔍 Latest deploy log https://app.netlify.com/sites/eclectic-pie-88a2ba/deploys/678ef1b3e9cc480008a98331
😎 Deploy Preview https://deploy-preview-581--eclectic-pie-88a2ba.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@calvincestari
Copy link
Member Author

calvincestari commented Jan 21, 2025

Only thing I'm not clear on is whether the deferredFragments property should still be declared as public when the module type is embeddedInTarget(accessModifier: .internal). Will the executor still be able to access it if not?

@calvincestari
Copy link
Member Author

We should probably also get @defer into an operation in one of our internal codegen reference projects like AnimalKingdom.

Copy link
Contributor

@AnthonyMDev AnthonyMDev 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 this fix!

@calvincestari calvincestari merged commit 3294d83 into main Jan 22, 2025
36 checks passed
@calvincestari calvincestari deleted the fix/defer-metadata-extensions-embedded-in-target branch January 22, 2025 20:59
BobaFetters pushed a commit that referenced this pull request Jan 22, 2025
BobaFetters pushed a commit to apollographql/apollo-ios-codegen that referenced this pull request Jan 22, 2025
BobaFetters pushed a commit that referenced this pull request Jan 22, 2025
e4f12df6 fix: Defer metadata extensions (#581)

git-subtree-dir: apollo-ios-codegen
git-subtree-split: e4f12df6f713fd15c19e734c9c6300fb0c1e4db5
BobaFetters pushed a commit that referenced this pull request Jan 22, 2025
git-subtree-dir: apollo-ios-codegen
git-subtree-mainline: 7d863f5
git-subtree-split: e4f12df6f713fd15c19e734c9c6300fb0c1e4db5
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.

bug: Defer metadata extension generated incorrectly for module types embeddedInTarget and other
3 participants