Fix default model detection when using other directives combination with @paginate

[spawnia]

• Added or updated tests
• Added Docs for all relevant versions
• Updated the changelog

Resolves #550
Supersedes #949

Changes

This now works:

type Query {
  users: [User!]!
    @can(ability: "adminOnly")
    @paginate
}

When creating the intermediary pagination type the original class of the model is attached through the new @modelClass directive.

Since @modelClass should be able to replace pretty much all instances where a custom model argument was needed, i decided to deprecate the current @model directive.

I modified @node so it now also handles what @model did before and users have an easy migration path.

-type User @model {
+type User @node {
  id: ID
}

The plan is to rename @modelClass to @model in v5 and make it the new standard for mapping types to models. Let's hold off on pushing it onto users for now.

Breaking Changes

No. Until v5, @model will function as before. When using @node, a custom resolve argument will take precedence over the new default model resolution that was added.

[lorado]

Ok, this PR is pretty huge, but refactoring makes sense.
Great job. I still have some small questions... I'll post them in PR later

[lorado]

Ooooohhhh I just got it... you created a directive, that maps a WHOLE TYPE to a class, not just a return type of a field. Dude, this is really awesome! :D

Should we maybe collect somewhere changes, that has to be done in v5? like renaming @modelClass to @model, removing model from all directives and ignoring it on resolving the class?

[spawnia]

Should we maybe collect somewhere changes, that has to be done in v5?

What i usually do is to just go through and look for all the places that have something marked as deprecated

like renaming @modelClass to @model

Plan to do that, yes.

removing model from all directives and ignoring it on resolving the class?

I think that will have to wait until v6. I want to allow for a gradual migration, straight from the model argument to @model. As the renaming is pending, i would like to wait until v5 to even suggest that change.

[lorado]

Ok fine, then let's merge it. YEY

[JasonTheAdams]

Overall looks pretty good. There's a lot of extra refactoring that got snuck in this PR (e.g. changing how the tests work), but the changes look good.

The only thing that stands out to me is the usage of the DocumentAST class. It has three public array properties, and two setters. For the sake of protecting the integrity of those array elements, I would suggest making getters, setters, and isset methods for the three properties, and then making those properties private. This also allows you to reconcile the exceptions. You really don't want other code modifying the AST directly without a method abstraction.

Hope this helps! 😄

[spawnia]

There's a lot of extra refactoring that got snuck in this PR (e.g. changing how the tests work)

True, thanks for digging through. I tend to go a bit overboard with the boy scout rule.

The only thing that stands out to me is the usage of the DocumentAST class.

Let's continue that discussion in #986

Thanks @lorado and @JasonTheAdams for your review, it is really valuable to have you validate my work.