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

Implement new naming schemes #96

Merged
merged 33 commits into from
Feb 29, 2020
Merged

Implement new naming schemes #96

merged 33 commits into from
Feb 29, 2020

Conversation

comigor
Copy link
Owner

@comigor comigor commented Feb 27, 2020

From comments on #90 and #93:

The current pathed naming approach (let's call it pathedWithTypes) has some corner cases despicted on #93 #63 #91. If the path to the same object forks, but use the same classes on both forks, it'll be unable to generate two (or more) classes with the same name. So, in this case, you'd have to use aliases.

This PR adds a new option to choose which kind of class names will be generated: pathed with field names, pathed with type (GraphQL classes) names and "simple" (do not use pathes).

  • pathedWithTypes: default, where the names of previous types are used as prefix of the next class. Eg. QueryName$QueryRoot$Pokemon$Pokemon
  • pathedWithFields: the names of previous fields are used as prefix of the next class. Eg. QueryName$QueryRoot$Charmander$Evolutions
  • simple: considers only the actual GraphQL class name. This will probably lead to duplication and an Artemis error unless user uses aliases. Eg. Pokemon

For compatibility reasons, pathedWithTypes is still the default. But, theoretically, pathedWithFields is the one that would not generate conflicts. We can change the default (or even kill pathedWithTypes in the future). I'd still give the option to generate simple given some users like to use import aliases to prefix their generated code (and it gives the flexibility to not generate those huge names).

In the future, I'd like to generate the canonical (scalars, input objects and enums) objects into a different file.

@comigor comigor mentioned this pull request Feb 27, 2020
@comigor comigor changed the base branch from feat/long-naming-improvements to beta February 27, 2020 20:49
void main() {
group('On complex input objects', () {
test(
'Unused input objects will be filtered out',
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remember correct in previous versions there was no need to filter unused input and enum why are we going implement approach that requires filtering?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because I've created a new canonical visitor (responsible for retrieving all canonical classes: enums, input objects and fragments). However, not every of those canonical objects are used on each one of the queries, so I'm filtering out the unused values while we don't implement a single place to output canonical objects.

@vasilich6107
Copy link
Collaborator

vasilich6107 commented Feb 28, 2020

The overall solution seems to work fine. On monday we are starting the flutter project from scratch. I'll try to use this branch to generate sources.
After some trial period I'll write a second article on how to use all the GraphQL stack in Flutter

@vasilich6107
Copy link
Collaborator

vasilich6107 commented Feb 28, 2020

What do you think about omitting the Mixin postfix in pathedWithFields scheme? - https://tppr.me/vfrBS

Let me explain in details.

I'm thinking about usage of the generated types in project. On the screenshot that I posted above you can see that we have three classes generated with the same ThingIdMixin.

The long named classes is a must to avoid the conflicts as we agreed. But mixins could be useful for referring to the specific types. For example I can define a widget that accepts ThingIdMixin as parameter. This will help to avoid long names polluting the project code base.

As far as I see there is no any convention that recommends to use Mixin postfix in mixin naming.
Here is an examples of sources:

https://dart.dev/guides/language/language-tour#adding-features-to-a-class-mixins
https://resocoder.com/2019/07/21/mixins-in-dart-understand-dart-flutter-fundamentals-tutorial/
https://alligator.io/dart/mixins/

Also we did not postfix the classes with Class :-)
Mixin postfix did not help us with uniqueness.

Here is an example in code usage

var func = (ThingId thing) => thing.id
var func1 = (UserData user) => user.name
var func2 = (Employee employee) => employee.name

and

var func = (ThingIdMixin thing) => thing.id
var func1 = (UserDataMixin user) => user.name
var func2 = (EmployeeMixin employee) => employee.name

Mixin looks a little bit redundant.
First example without Mixin looks shorter and more elegant)

@vasilich6107 vasilich6107 mentioned this pull request Feb 28, 2020
@comigor
Copy link
Owner Author

comigor commented Feb 29, 2020

Sure, there's not needed to call it mixin, I'll update with those changes as well

@comigor
Copy link
Owner Author

comigor commented Feb 29, 2020

Actually, it seems the following is possible:

type Person {
    id: ID!
    firstName: String!
    lastName: String!
}
fragment Person on Person {
    firstName
    lastName
}

query {
    personById(id: "abc") {
        ...Person
    }
}

And I couldn't find anything on spec on validation of name uniqueness between fragments definitions and other objects, only the fragment declaration itself: https://spec.graphql.org/June2018/#sec-Fragment-Name-Uniqueness.

So, if we don't append something to the fragment definition, it's possible for it to collide with some other declaration name.

As this is the beta branch, I'm gonna merge it and we can still think about this and open another PR later. This one is too huge already!

@comigor comigor merged commit d2646f0 into beta Feb 29, 2020
@comigor comigor deleted the local/naming branch February 29, 2020 05:02
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.

None yet

2 participants