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

API: Customise type names and operation names #143

Merged

Conversation

unclecheese
Copy link

Resolves #95

@flamerohr flamerohr mentioned this pull request Mar 12, 2018
2 tasks
@unclecheese unclecheese force-pushed the pulls/1/insane-in-the-typename branch from 583eaaa to 87fce2b Compare March 12, 2018 21:49
@unclecheese unclecheese changed the base branch from 1 to master March 12, 2018 21:50
@unclecheese unclecheese force-pushed the pulls/1/insane-in-the-typename branch 2 times, most recently from b4d7191 to a0bd07c Compare March 12, 2018 22:14
@tractorcow
Copy link
Contributor

@unclecheese please rebase on 1 branch. master is for framework 5.x compat, and we want this in 4.2

Copy link
Contributor

@tractorcow tractorcow left a comment

Choose a reason for hiding this comment

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

Please rebase (see comment)

@tractorcow
Copy link
Contributor

Change of plan, we're branching master into 2 and will adjust master to 3.x (for framework 5.x compat).

@unclecheese unclecheese changed the base branch from master to 2 March 19, 2018 23:03
@tractorcow
Copy link
Contributor

The new API seems very out of sync with the existing scaffolding config. For instance, this is how a type of MyProject\Post is scaffolded into a set of operations.

SilverStripe\GraphQL\Controller:
  schema:
    scaffolding:
      types:
        MyProject\Post:
          fields: [ID, Title, Content]
          operations:
            read: true
            create: true

Why isn't typeName simply a property on one of these nested values, and instead is shifted to the SilverStripe\GraphQL\Scaffolding\Schema.schema config? You could even decorate the existing structure for operations allowing extended config for specific actions. E.g.

SilverStripe\GraphQL\Controller:
  schema:
    scaffolding:
      types:
        MyProject\Post:
          type: MyPost
          fields: [ID, Title, Content]
          operations:
            read:
              type: readPosts
            create:
              type: createPost

At the moment we are branching a new major version specifically for a simple class rename and shifting a static accessor into an instance method. For a new major version I would like to see the API break in a way that promotes cohesion rather than additional fragmentation.

@tractorcow
Copy link
Contributor

In addition, we haven't got any similar support for the php api scaffolding.

Could we enable object / operation naming as such?

$scaffolder
            ->type(Post::class)
                ->setTypeName('MyPost')
                ->addFields(['ID', 'Title', 'Content'])
                ->operation(SchemaScaffolder::READ)
                    ->setTypeName('readMyPosts')
                    ->end()
                ->operation(SchemaScaffolder::UPDATE)
                    ->setTypename('updateMyPost')
                    ->end()
                ->end();

@unclecheese
Copy link
Author

Summary from offline discussion with @tractorcow:

  • The reason for the API breakage is the change in typenames. The Schema was just a bonus refactoring given the precondition of API breakage
  • Custom operation names can be inlined with setName(). They do not have to be guaranteed unique, because they could be nested. If however, there are two operations with the same name at the root query level, we have no collision detection in place for that. It would be a Manager exception.
  • We cannot inline custom type names, e.g. setTypeName('MyType') due to the declarative nature of the scaffolding. Nested dependencies on the typenames get declared, and the order of execution then becomes critically important, e.g.:
$scaffolder->type(BlogPost::class)
  ->fields(['ID', 'Author']) // author now exposed. its typename must be deterministic
  ->end()
  ->type(Member::class)
     ->setTypeName('MySpecialMember') // oh no!
class Manager {
   public static function createFromConfig($config) {
      Schema::inst()->setTypeNames($config['typeNames']);
   }
}

@tractorcow
Copy link
Contributor

Yep turns out it's not that easy is it! You're right we do need to pre-define this mapping before we can scaffold anything.

@tractorcow
Copy link
Contributor

Also please fix up the docs on the yml / php methods for naming of operations. Make sure the YML examples you give include the entire config tree, not just a subtree, since users reading it won't know

My\Really\Long\Namespaced\DataObject:
  operations:

needs to be nested inside

SilverStripe\GraphQL\Controller:
  schema:
    scaffolding:
      types:

@tractorcow
Copy link
Contributor

tractorcow commented Mar 20, 2018

@unclecheese a question about subclasses.

Lets say we scaffold Blog as it's own type.

If someone subclasses Blog, can we still query those objects using readBlog? typeNameForDataObject('SubBlog') wouldn't be 'Blog' would it?

@tractorcow
Copy link
Contributor

Ok I've pushed up what I think gets this over the line:

  • Schema -> StaticSchema (so it won't conflict with the other class)
  • Updated docs
  • Fixed a bug
  • Changed bootstrapping method (no longer has own config key)

@tractorcow
Copy link
Contributor

Note: Tests are all messed up. A problem for tomorrow. :)

@tractorcow tractorcow force-pushed the pulls/1/insane-in-the-typename branch from f33dd36 to f8d63b9 Compare March 20, 2018 23:09
@tractorcow
Copy link
Contributor

Merged 1 -> 2, 2 -> master, rebased this on 2. Some errors surfaced. Am looking into them.

@tractorcow tractorcow force-pushed the pulls/1/insane-in-the-typename branch from 727f3ba to 28df271 Compare March 20, 2018 23:28
@tractorcow tractorcow merged commit 87afe84 into silverstripe:2 Mar 20, 2018
unclecheese pushed a commit to unclecheese/silverstripe-graphql that referenced this pull request Jan 27, 2021
…ing-warning-warning

Fix linting warnings in sass
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