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

NEW: Compatibility with GraphQL 4 changes #870

Merged
merged 6 commits into from
Feb 17, 2021

Conversation

unclecheese
Copy link

Summary

Use new context provider API. No more BuildState.

Depends on silverstripe/silverstripe-graphql#352

@unclecheese
Copy link
Author

Confirmed failures are due to missing UserContextProvider class.

@chillu chillu merged commit 43842fc into silverstripe:4 Feb 17, 2021
@tractorcow
Copy link
Contributor

This change just broke compatibility with graphql 3. Do you want to fix the composer dependencies to reflect that 3 is no longer supported?

@tractorcow tractorcow mentioned this pull request Jul 1, 2021
@unclecheese
Copy link
Author

Shouldn't have. We test everything in both v3 and v4. https://travis-ci.com/github/silverstripe/silverstripe-elemental/jobs/519607824

What issue are you seeing?

@tractorcow
Copy link
Contributor

tractorcow commented Jul 1, 2021

my typeNames is no longer respected due to the removal of
return StaticSchema::inst()->typeNameForDataObject(static::class);

Which is ok; You don't need to support both versions simultaneously; we just need to make sure that we aren't incorrectly treating this module as graphql 3 compatible.

@tractorcow
Copy link
Contributor

I guess my attempt to upgrade a framework 4.7 to 4.8 can't be done unless I upgrade to graphql 4 huh. Oh well.

@unclecheese
Copy link
Author

We are supporting both versions simultaneously. The method you're referring to still exists in GraphQL 3, right? Can't you just run GraphQL 3?

@tractorcow
Copy link
Contributor

tractorcow commented Jul 1, 2021

Yes, the method exists, and elemental no longer calls it, when it used to, and still needs to.

now it just assumes the below is correct.

return str_replace('\\', '_', static::class);

So with graphql config as below:

      typeNames:
        myProject\HeroBlock: HeroBlock

I get graphql results as below: (elemental 4.6)

"node": {
      "ID": "6",
      "BlockSchema": {
        "typeName": "myProject_HeroBlock",
        "title": "About the collection / Mō te kohinga",
        "showTitle": 1
  }
}

when it should be (elemental 4.5)

"node": {
      "ID": "6",
      "BlockSchema": {
        "typeName": "HeroBlock",
        "title": "About the collection / Mō te kohinga",
        "showTitle": 1
  }
}

@unclecheese
Copy link
Author

Right, OK.. The "backward compatibility" is focussed on the CMS, so there may still be upgrade issues in your frontend app. I think this is really as simple as adding a fork in the typeName function that uses the old GraphQL 3 method if it exists, right? Can we fix this in two lines of code?

@unclecheese
Copy link
Author

    public static function getGraphQLTypeName(): string
    {
        return class_exists(StaticSchema::class)
            ? StaticSchema::inst()->typeNameForDataObject(static::class)
            : str_replace('\\', '_', static::class);
    }

@unclecheese unclecheese deleted the pulls/4/nuclear-refactor branch July 1, 2021 23:20
@tractorcow
Copy link
Contributor

That'd probbably do it!

Sorry, i've been out of the silverstripe space for a while, not sure what the state of play is with the recent versions. :)

@tractorcow
Copy link
Contributor

Thanks for the quick response too. I don't need a fix urgently, I just wanted to report back my findings in case it was useful for you. I'm happy to keep my site on 4.7 in the mean time.

@tractorcow
Copy link
Contributor

convention over configuration is the way to go by the way. graphql 4 is very cool.

@unclecheese
Copy link
Author

Thanks! If you like convention over configuration, you'll love what we're doing with NextJS. :-)

@unclecheese
Copy link
Author

Can you just apply that patch locally and let me know if it works, and if so, I'll just make a PR now.

@tractorcow
Copy link
Contributor

Will do, 1 moment.

@tractorcow
Copy link
Contributor

looks good 👍

"node": {
                "ID": "6",
                "BlockSchema": {
                  "typeName": "HeroBlock",

@tractorcow
Copy link
Contributor

When I get some time, I'll just upgrade to graphql 4 (that's the real fix here, I have to admit).

@unclecheese
Copy link
Author

PR is up #912

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.

3 participants