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

DEP Use graphql 3 #851

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Nov 17, 2020

Use graphql 3.x-dev when installing the 4.x-dev recipe

Specify graphql 3 in composer.json

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Why don't you constrain this release line to GraphQL 3, then release a new minor elemental version which works on GraphQL 4?

@emteknetnz
Copy link
Member Author

emteknetnz commented Nov 17, 2020

The issue is with silverstripe/recipe-cms:4.x-dev which we install on .travis.yml, not elemental itself so I don't want to touch this module

Travis currently has a backlog of builds to run, though this branch is green on creative-commoners travis account https://travis-ci.com/github/creative-commoners/silverstripe-elemental/builds/201924599

@robbieaverill
Copy link
Contributor

Elemental does have a direct dependency on GraphQL though. I'm not as familiar with the decisions made here, but it does seem to me like this client-level abstraction you're proposing is just going to become a nightmare to maintain.

@emteknetnz
Copy link
Member Author

Elemental requires versioned-admin which requires graphql is how things currently work.
We're currently mid release so I do not want to touch composer.json requirements right now

Graphql 4 compatibility in core modules was merged on 1.x-dev/4.x-dev branches in the last few days which has complicated travis builds for matrix's that install recipe 4.x-dev for the following modules:

  • silverstripe-elemental
  • silverstripe-versioned-admin
  • silverstripe-asset-admin

client-level abstraction you're proposing is just going to become a nightmare to maintain

I don't think it will? At this stage we're only expecting to have to update 3 travis files, and we'll probably be migrating to a shared travis config within the month which will make this sort of thing even easier

@robbieaverill
Copy link
Contributor

Ok, when 4.8 is released what are you intending to do with this change? In my mind if we're supporting GraphQL 3 and 4 then it'd be worth running tests on both versions - at least for behat

@emteknetnz
Copy link
Member Author

I'm not sure because I don't know if graphql will have a stable tag then. We will have the option of removing it because elemental will be compatible because of these unreleased commits on elemental 4.x-dev

image

If we wish to test both, then we could simply create another matrix entry to test on graphql 3 - silverstripe-admin does this now

image

@emteknetnz
Copy link
Member Author

Chatted to @Cheddam about this, we've agreed to change this to making the change in composer.json

@emteknetnz emteknetnz changed the title MNT Use graphql 3 DEP Use graphql 3 Nov 17, 2020
@emteknetnz
Copy link
Member Author

Travis doesn't appear to be working on this repo/account?

Travis is green on creative-commoners - https://travis-ci.com/github/creative-commoners/silverstripe-elemental

@Cheddam Cheddam merged commit 47f2fc5 into silverstripe:4.5 Nov 17, 2020
@Cheddam Cheddam deleted the pulls/4.5/graphql3 branch November 17, 2020 03:11
chillu added a commit to open-sausages/silverstripe-elemental that referenced this pull request Jan 18, 2021
This was originally added to the 4 release branch,
and then removed prior to the 4.5.0 release through
silverstripe#851
to simplify Travis builds.

Explicitly declaring the module *doesn't* support GraphQL 4
is incorrect, and hinders early adoption - you'll need to
run a "silverstripe/graphql:4.x-dev as 3.x-dev" dependency.

It's also a different strategy compared to e.g. versioned-admin:
silverstripe/silverstripe-versioned-admin@cae464f
@chillu
Copy link
Member

chillu commented Jan 18, 2021

I've sent a follow up to restore GraphQL 4 support: #865

@emteknetnz Please ping @unclecheese or myself when you make those kinds of changes, when there's a significant overlap with our work (and it reverts an earlier change by Aaron).

chillu added a commit to open-sausages/silverstripe-elemental that referenced this pull request Jan 19, 2021
This was originally added to the 4 release branch,
and then removed prior to the 4.5.0 release through
silverstripe#851
to simplify Travis builds for the release activity.

Explicitly declaring the module *doesn't* support GraphQL 4
is incorrect, and hinders early adoption - you'll need to
run a "silverstripe/graphql:4.x-dev as 3.x-dev" dependency.

It's also a different strategy compared to e.g. versioned-admin:
silverstripe/silverstripe-versioned-admin@cae464f

Relies on silverstripe/silverstripe-travis-shared#4
emteknetnz pushed a commit that referenced this pull request Jan 19, 2021
* DEP Restore GraphQL 4 support

This was originally added to the 4 release branch,
and then removed prior to the 4.5.0 release through
#851
to simplify Travis builds for the release activity.

Explicitly declaring the module *doesn't* support GraphQL 4
is incorrect, and hinders early adoption - you'll need to
run a "silverstripe/graphql:4.x-dev as 3.x-dev" dependency.

It's also a different strategy compared to e.g. versioned-admin:
silverstripe/silverstripe-versioned-admin@cae464f

Relies on silverstripe/silverstripe-travis-shared#4

* DEP Change to recipe-testing

See #865 (review)
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