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

refactor: extracted DataDict.asJSONDictionary() #377

Conversation

TizianoCoroneo
Copy link
Contributor

Hey there 👋

recently I found the need to encode a DataDict instance into a JSON object. I found that this functionality is already provided but only for GraphQLResult instances instead.

I refactored the relevant code to also expose a DataDict.asJSONDictionary() method that performs the same logic as the one in GraphQLResult.

Copy link

netlify bot commented May 30, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2cf5f28

Copy link

netlify bot commented May 30, 2024

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2cf5f28

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 the PR @TizianoCoroneo!

The team discussed this and we'd like to request a change before merging this. The DataDict is not really intended for public consumption. It has to be public in order for your generated models to access it and compile properly, but we don't want to encourage people to use it directly.

We think this would be improved by making this accessible directly from SelectionSet instead of DataDict.

We also try not to cloud the namespace of SelectionSet too much because of naming collisions and code completion. I haven't talked to the team about this part yet, but I'm wondering if it would be better to have a global function or static struct for this. Something like Apollo.JSONConverter.convert(_ selectionSet: ApolloAPI.SelectionSet) -> [String: Any].

We definitely want to do this on SelectionSet, not DataDict, but lets discuss the actual API for this before finalizing and merging.

@calvincestari @BobaFetters, any opinions/suggestions?

@calvincestari
Copy link
Member

I'm wondering if it would be better to have a global function or static struct for this. Something like Apollo.JSONConverter.convert(_ selectionSet: ApolloAPI.SelectionSet) -> [String: Any].

I hadn't considered the package namespace for this but I'm not opposed to it. I think it could help with clarity on where this feature is available.

@BobaFetters
Copy link
Member

Yea I like the idea of having available as a global like you suggested
Apollo.JSONConverter.convert(_ selectionSet: ApolloAPI.SelectionSet) -> [String: Any]

And I think we should use the JSONConverter to hold all of our helper functions that handle JSON conversion, maybe we go through in future PRs and deprecate some existing functions and bring it all into one place.

@TizianoCoroneo
Copy link
Contributor Author

I updated the PR. Now there is an enum with static methods to perform the conversions like this:

let jsonObject = Apollo.JSONConvert.selectionSetToJSONObject(myHero)

I also added a function to convert a GraphQLResult into a JSONObject that internally uses asJSONDictionary, just because the sight of one empty enum with one static method kind of hurts.

@AnthonyMDev AnthonyMDev mentioned this pull request Jun 5, 2024
@AnthonyMDev
Copy link
Contributor

I've branched off of this PR and made a few changes. New PR is up #380. Thanks for the help @TizianoCoroneo!

@AnthonyMDev AnthonyMDev closed this Jun 5, 2024
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.

4 participants