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

Add graphql.schemaPath configuration #8773

Merged
merged 4 commits into from Aug 21, 2023

Conversation

marekryb
Copy link
Contributor

@marekryb marekryb commented Aug 17, 2023

This pull requests adds two new configuration options:

  • graphql.schemaPath
    Allows you to define path of generated GraphQL schema (default: schema.graphql)
    Motivation:
    Quite commonly I create custom mutations through extendGraphqlSchema because I need to use transaction over multiple tables and/or increment/decrement counter. In these situations, I would like to completely disable some operations through graphql: { omit: { create: true, update: true } } on these lists, but for sake of having Admin UI working, I cannot do this. This sometimes causes confusion among fellow developer(s) or a 3rd party as they start using some mutations that are not intended to be ever used and later they have to change it over.
    In all recent projects I am using keystone as a route in nextjs (as in examples/framework-nextjs-*-directory) so I do not need Admin UI in production. In development however, Admin UI is commonly preferred over direct database access. So what I actually need is to have two GraphQL schemas: one used inside nextjs (no AdminUI) and one for standalone server (with AdminUI).

- graphql.excludeKeystoneMeta
Allows you to exclude keystone: KeystoneMeta! query from the GraphQL API that is used by the Admin UI.
This is seems reasonable in cases where AdminUI is disabled (either through ui.isDisabled: true or because using it within framework like nextjs). Initially I was thinking to use ui.isDisabled as a condition for dropping the query rather than new config option, but this would be breaking change with no way to opt-out.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 17, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cdb9427:

Sandbox Source
@keystone-6/sandbox Configuration

@dcousens dcousens self-requested a review August 17, 2023 04:04
@dcousens dcousens self-assigned this Aug 17, 2023
@dcousens
Copy link
Member

dcousens commented Aug 17, 2023

@marekryb this is awesome!

For graphql.excludeKeystoneMeta, I am happy to make that drop with ui.isDisabled, as we are already on a breaking change merge cycle - but I guess my question is, how problematic are the set of currently merged breaking changes for you.

We are actually already looking similar breaking changes for this in #8765

@marekryb marekryb changed the title Add graphql.schemaPath and graphql.excludeKeystoneMeta configuration Add graphql.schemaPath configuration, exclude KeystoneMeta when ui.isDisabled Aug 17, 2023
@marekryb
Copy link
Contributor Author

marekryb commented Aug 17, 2023

@dcousens Okay. I removed excludeMetaSchema and tied it to ui.isDisabled. Many tests are failing, but I am pretty sure it is not because of this PR.

I looked briefly at recent PRs and see no problem. 268d703 removes meta from sudo() which is fine. This PR goes further and removes it also from the final client schema.

I am confused though about what is happening in the repo. Eg. PR #8767 which was supposedly merged to main in a04d66d, but it says This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository. and I cannot see it on the main branch. WTH? I noticed this one specifically because it fixes test in a file that I am also modifying.

@dcousens
Copy link
Member

dcousens commented Aug 18, 2023

I am confused though about what is happening in the repo. Eg. PR #8767 which was supposedly merged to main in a04d66d, but it says This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository. and I cannot see it on the main branch. WTH? I noticed this one specifically because it fixes test in a file that I am also modifying.

Fair question!
We needed to do a patch release so we rebased the breaking change pull requests on top of the patch release, to try and keep the release history linear, but at the cost of the unreleased pull request history being rebased.

I think we might want to switch to a different git workflow when we're doing majors in the future.

@dcousens
Copy link
Member

dcousens commented Aug 20, 2023

@marekryb thinking about this, for future version updates, we'll endeavor to merge pull requests to a separate branch that will be rebase merged with changeset already run as part of a release.

This should set us up for a minimal amount of git work while leveraging the same pull request workflows we use now.

@marekryb
Copy link
Contributor Author

@dcousens Ok, thank you for explaining. Separate branch seems more natural indeed.
Let me know when/if I need to rebase or do some other work on this PR.

@dcousens
Copy link
Member

dcousens commented Aug 20, 2023

@marekryb we have reset the main branch to the tag 2023-08-15 and moved all of the existing breaking changes into a new branch, please see #877

This will allow us to keep main as non-breaking moving forward and resolve a few of our changesets/pnpm workflow issues.

@dcousens dcousens force-pushed the graphql_schemaPath branch 2 times, most recently from 3302f98 to f64d0fc Compare August 21, 2023 00:29
@dcousens
Copy link
Member

dcousens commented Aug 21, 2023

@marekryb we have removed any references to the excludeKeystoneMeta and ui.isDisabled changes from this pull request, and are going to merge them with our other changes in an upcoming pull request.

We will ensure to add your name as the following on the merge and commit messages for that upcoming pull request (and this), as we will be using your commits and idea 💛

    Co-authored-by: Marek Rybczynski <...@gmail.com>

(this is derived from the authorship information you put in your commits)

Co-authored-by: Marek Rybczynski <marek.rybczyn@gmail.com>
@dcousens dcousens changed the title Add graphql.schemaPath configuration, exclude KeystoneMeta when ui.isDisabled Add graphql.schemaPath configuration Aug 21, 2023
@dcousens dcousens merged commit 32f7a6a into keystonejs:main Aug 21, 2023
14 of 17 checks passed
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