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: Schema initialise task #433

Closed

Conversation

unclecheese
Copy link

@unclecheese unclecheese commented Jan 17, 2022

There's too much boilerplate in setting up a default schema. What this does:

  • Creates a _graphql dir with models.yml, types.yml, queries.yml, mutations.yml,
  • Creates a _config/graphql.yml file that routes and configures your default schema (Director rule, src: property)
  • Creates an empty Resolvers class where you can define custom resolvers
  • Registers the resolver class with schemaConfig.resolvers

Example

$ vendor/bin/sake dev/graphql/init namespace="MyAgency\MyApp"

Arguments

  • namespace: The root app namespace (required)
  • name: The schema name (default: default)
  • graphqlConfigDir: The folder where the flushless graphql files will go (default: _graphql/)
  • graphqlCodeDir: The subfolder of src/ where your GraphQL code (the resolver class) will go. Follows PSR-4 based on the namespace argument (default: GraphQL)

File permissions

Inherits whatever you're using on the app/ directory.

It's idempotent

Any of the above artefacts that already exist will not be over written, and it will tell you that.

Related PRs

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Concept seems OK? Feels like it needs more work around CLI documentation and experience

src/Dev/Initialise.php Outdated Show resolved Hide resolved
src/Dev/Initialise.php Outdated Show resolved Hide resolved
src/Dev/Initialise.php Show resolved Hide resolved
{
$absConfigFile = Path::join(BASE_PATH, $this->projectDir, '_config', 'graphql.yml');
if (file_exists($absConfigFile)) {
echo "Config file $absConfigFile already exists. Skipping." . PHP_EOL;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to suffix the config if the graphql.yml file already exists?

Copy link
Author

@unclecheese unclecheese Jan 27, 2022

Choose a reason for hiding this comment

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

Ideally, this would do a similar check that the GraphQL devtools controller does, where it finds what schemas are actually routed. That's in a separate module, and it's got some GraphQL 3 backward compat stuff in it. I don't really have much of an appetite for migrating it into GraphQL 4 right now, but we could at some point. Would probably make sense to put in Controller::getRoutedSchemas() or something.

I'm thinking that if you have a graphql.yml file, you're past the point of needing this tool? I'd rather be paranoid of clobbering someone's existing config.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good way of handling it - if the file already exists, we inform the developer of that fact and skip it. They can then make a judgement call to either keep the existing schema definition or delete it and run this again.

src/Dev/Initialise.php Show resolved Hide resolved
src/Dev/Initialise.php Outdated Show resolved Hide resolved
@@ -13,3 +13,7 @@ SilverStripe\GraphQL\Dev\DevelopmentAdmin:
controller: SilverStripe\GraphQL\Dev\Build
links:
build: Build the GraphQL schema
init:
Copy link
Member

Choose a reason for hiding this comment

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

Is there any docs that links to vendor/bin/sake dev/graphql/init? Right now it's very hidden

Copy link
Author

Choose a reason for hiding this comment

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

* matching the pattern resolve<FieldName> or resolve<TypeNameFieldName>
* will be automatically assigned to their respective fields.
*
* More information: https://docs.silverstripe.org/en/4/developer_guides/graphql/working_with_generic_types/resolver_discovery/#the-resolver-discovery-pattern
Copy link
Member

Choose a reason for hiding this comment

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

Lines too long, will fail PHPCS

Copy link
Author

Choose a reason for hiding this comment

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

The link itself is enough to break the limit. What would you recommend?

src/Dev/Initialise.php Outdated Show resolved Hide resolved
src/Dev/Initialise.php Show resolved Hide resolved
@unclecheese
Copy link
Author

Docs are here: silverstripe/silverstripe-framework#10218

@unclecheese
Copy link
Author

Revisions made per feedback.

@maxime-rainville
Copy link
Contributor

I'm wondering if maybe we want to move this to the GraphQL dev tools module instead? That way we can just call it experimental and merge it.

@GuySartorelli
Copy link
Member

I don't necessarily think that we should push it off to another module just to get it merged if it's something that we think should be in core at the end of the day... that said, this is ultimately something that helps at dev-time and shouldn't be used in a production environment (if I understand it correctly) so dev tools does genuinely seem to be a better place for it.

@unclecheese
Copy link
Author

unclecheese commented May 23, 2022

I don't see it as a good fit for dev tools. I'll try to explain why.

There are essentially two use cases for this module:

  • Make the CMS work
  • Provide a bespoke GraphQL schema for my app

In the case of the latter, your entry point is now creating a bunch of boilerplate yaml before you can request a single byte of data. The time when this feature is most useful is during that time. Your journey will start with the docs or a Slack message to get the tl;dr on how you do it . If we can deliver a single CLI task to get you into your schema in a few minutes rather than hours, we should not pass on the opportunity to provide that value. With devtools, by the time you've got your hands dirty and are seeking the features of that module, it's too late for this to be effective.

You could certainly make the case that if you're building a bespoke GraphQL server in Silverstripe CMS, you're highly unlikely to do it without the devtools module (imagine building a schema and not testing out any queries in the IDE!), and this is just another reinforcement of the criticality of that module when doing a custom GraphQL schema, but I think that speaks more to the case for renaming the devtools module to "silverstripe-graphql-custom-schema" or something -- in other words, a specific module that provides tools to break GraphQL out of the conceptual boundaries of the CMS, rather than framing it as a power-user addon.

I'm more wondering what's left to do on this PR? It's been stale for months, which is fine, but I don't think the fact that its been sitting for a while should be a pretext for changing its intended domain.

@GuySartorelli
Copy link
Member

Your journey will start with the docs or a Slack message to get the tl;dr on how you do it .
...
With devtools, by the time you've got your hands dirty and are seeking the features of that module, it's too late for this to be effective.

This seems to me like it's ultimately a matter of documentation - if the documentation says something along the lines of "An easy way to get started creating your schema is using the schema initialise task in the devtools module" with a link to that module, then it brings not only this functionality but also the dev tools module in general to the attention of people who are just starting out with making their custom graphql schema.

I'm more wondering what's left to do on this PR?

There were a few discussion points that someone just needs to make a call on, I think - maybe @emteknetnz could take another look now that his questions have been answered.

I don't think the fact that its been sitting for a while should be a pretext for changing its intended domain.

Agreed - but now that the question has been asked I think it does behoove us to decide if this code might be better suited to devtools. If the only reason for it not to be there is that we want to make people aware of it, I think that's a matter of documentation as I mentioned above. In general it would be better to keep code intended for use at development time in a module intended to be a dev dependency.

@unclecheese
Copy link
Author

The thing is, 90% of the code in this module is not intended for production. The whole thing is a giant task that creates an artefact that is intended for production. So shipping dev code is part of the purchase at this point.

Reading a "Quick Start" documentation for a module and getting told that you need to install another module to do the quick start is probably a good indication that we've crippled the DX. I just don't see any positive outcomes from that.

@emteknetnz emteknetnz changed the base branch from master to 4 May 23, 2022 05:13
@maxime-rainville
Copy link
Contributor

Yeah ... to me this sounds more like a documentation concern. Having a quick way to scaffold a graphql schema is pointless if you don't also have a quick way to run a dummy query against it. The doc should push you to install the dev tools early.

On a side note, we probably need to start providing more formal support for those dev-tools.

@unclecheese
Copy link
Author

In theory, the devtools module shouldn't have to exist. The graphql module is itself, for devs. It produces a single deployable artefact. Really everything but the controller could be left out of the deployment. So it makes sense that the IDE and all of its furnishings should ship with the module.

It was this way for a short time, until we realised that GraphQL Playground came with massive JS bundle that bloated this repository. We decided to prune the commits and move the playground to a separate module. That minor detail is what put us on this path of two separate modules, and it's never really felt right.

So I guess I'm fine with it either way. I think I'm just a bit annoyed that devtools has to exist. It feels like unnecessary documentation and maintenance overhead. But here we are. 🤷‍♂️

@emteknetnz
Copy link
Member

Closed in favor of silverstripe/silverstripe-graphql-devtools#53

@emteknetnz emteknetnz closed this Apr 10, 2024
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.

5 participants