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

feat: Add support for custom scalar validation #3376

Merged
merged 11 commits into from
Aug 9, 2023

Conversation

bboure
Copy link
Contributor

@bboure bboure commented Jul 29, 2023

This PR fixes the following issues:

  • In GraphQL, optional input variables and types are also nullable.
  • in the JSON Schema, enum should not be combined with type (removed)
  • custom scalars should only accept basic primitives (i.e. no objects, arrays, etc)

@changeset-bot
Copy link

changeset-bot bot commented Jul 29, 2023

🦋 Changeset detected

Latest commit: e4a3d28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
graphql-language-service Minor
monaco-graphql Minor
cm6-graphql Patch
codemirror-graphql Patch
@graphiql/react Patch
graphiql Patch
graphql-language-service-cli Patch
graphql-language-service-server Patch
@graphiql/plugin-code-exporter Patch
@graphiql/plugin-explorer Patch
vscode-graphql Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for graphiql-test ready!

Name Link
🔨 Latest commit e4a3d28
🔍 Latest deploy log https://app.netlify.com/sites/graphiql-test/deploys/64cb84828e310100075d6c2b
😎 Deploy Preview https://deploy-preview-3376--graphiql-test.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Merging #3376 (57227f0) into main (2348641) will increase coverage by 0.10%.
Report is 4 commits behind head on main.
The diff coverage is 98.38%.

❗ Current head 57227f0 differs from pull request most recent head e4a3d28. Consider uploading reports for the commit e4a3d28 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3376      +/-   ##
==========================================
+ Coverage   55.75%   55.85%   +0.10%     
==========================================
  Files         110      110              
  Lines        5243     5256      +13     
  Branches     1426     1432       +6     
==========================================
+ Hits         2923     2936      +13     
  Misses       1897     1897              
  Partials      423      423              
Files Changed Coverage Δ
...nguage-service/src/utils/getVariablesJSONSchema.ts 94.59% <98.38%> (+0.71%) ⬆️

@acao
Copy link
Member

acao commented Jul 29, 2023

awesome work! can you add a changeset and another test case for the null case that isn't covered? the coverage info you can see on the files tab

@bboure bboure force-pushed the fix-variableJSONSchema-optional-input branch from 3fe61f6 to 042f103 Compare July 29, 2023 16:54
@acao
Copy link
Member

acao commented Jul 29, 2023

@bboure why don't you go ahead and add the custom mappings in this PR then, I agree it makes sense.

Others have expressed this need, and even introspecting the scalar info doesn't give us a deterministic solution. perhaps we can even provide a default mapping that can be overridden by users? Or perhaps we should provide none to avoid confusion.

Bonus if you can add the new scalar mapping to monaco-graphql as well, but if not I can. The main thing to remember is that the inter process boundary between main and worker process means the mapping will need to be json serializeable, though you don't need to perform the serialization.

it just means we will need a flat object mapping rather than a callback function or some such

Also, be sure to update the changelog and readme to reflect the changed behaviors with scalars

@acao
Copy link
Member

acao commented Jul 29, 2023

by an flat object "mapping", I mean that it could look like this, as I'm sure you were also thinking:

{ 
  DateTime: { type: "string", format: "date-time" }
}

come to think of it, i made a mapping just like this for a proprietary project where we declaratively generated openapi3 specs from graphql schema, I'm embarassed I didn't provide it sooner!

@bboure
Copy link
Contributor Author

bboure commented Jul 29, 2023

@acao ok, I'll give it a shot.

I'll give a shot at adding it to monaco-graphql since this is something that I'll need, but I might need some guidance.

I was thinking in that direction too.

I think one good option would be to add an option somewhere in DiagnosticSettings. We'll probably need a config per model uri.

Something like this

monacoGraphQLAPI.setDiagnosticSettings({
  validateVariablesJSON: {...},
  customScalarSchemaMappings: {
    [Uri.file('operation.graphql').toString()]: {
      DateTime: { type: "string", format: "date-time" },
      // maybe a short-hand for type could be useful but not strictly necessary as it might bring confusion too.
      MyCustomScarlar: 'string',
    }
  }
});

@acao
Copy link
Member

acao commented Jul 29, 2023

@bboure awesome! I think this could work fine, but I wonder if configuring it per schema makes even more sense?

initializeMode({
  schemas: [
    {
      uri: 'https://my-schema.com',
      fileMatch: ['**/*.graphql'],
      customScalarMappings: {
          DateTime: { type: "string", format: "date-time" },
          // maybe a short-hand for type could be useful but not strictly necessary as it might bring confusion too.
          MyCustomScarlar: 'string',
      }
    },
  ],
});

@acao
Copy link
Member

acao commented Jul 29, 2023

also, in case you're wondering why this lives on graphql-language-service, I have plans to use this logic for variables validation/completion in the vscode extension as well :)

@bboure
Copy link
Contributor Author

bboure commented Jul 29, 2023

@acao I added custom scalar schemas support and monaco-graphql config as suggested.

I also took the opportunity to improve the schema descriptions a bit.
Descriptions will now look as follow:

[field description] (for object types and if field description is present)
(two new lines)
Type (scalar or custom)
[type description] (If present, except for default scalars)

Note about type descriptions: I kept backward compatibility and do not render the description of default scalars (String, Int, etc). (They were probably removed for a reason, probably because they are obvious?)

For custom scalars, we use the description from the custom schema, if present.

Examples

The birth date of the user.

Date!
An ISO_8601 date
The name of the user

String!

lmk if you have comments

@acao
Copy link
Member

acao commented Jul 30, 2023

@bboure nice, and input objects retain their formatting as well?

@bboure
Copy link
Contributor Author

bboure commented Jul 30, 2023

the above applies to all types. Objects might look like this

A field description

MyObjectType!
My object description

@acao
Copy link
Member

acao commented Jul 30, 2023

hm,,. testing this out in vscode and monaco-graphql, it seems that something breaks and the hover no longer renders, I will need to see if something has regressed elsewhere

update: the issue is with regressions in the example projects from what I can tell

@acao
Copy link
Member

acao commented Jul 30, 2023

another issue with monaco is the schema url, the warning blocks validation messages

can you change the meta schema to draft 4? this is what package.json and many standards use, and is mostly all we need for the simple schemas we generate from variable definitions
'http://json-schema.org/draft-04/schema'

@acao
Copy link
Member

acao commented Jul 30, 2023

I have some other changes I was planning to add today, for example to add optional anchor links for the graphiql@4 release with monaco coming soon, but I will wait until this PR is complete

@bboure
Copy link
Contributor Author

bboure commented Jul 30, 2023

update: the issue is with regressions in the example projects from what I can tell

Where is the issue exactly?

did I break something?

@bboure
Copy link
Contributor Author

bboure commented Jul 30, 2023

I tried this in one of my projects. it seems to work fine

image

@acao
Copy link
Member

acao commented Jul 31, 2023

did I break something?

no, there is a regression in the demos, where someone changed the mode instantiation pattern, so when the schema is fetched, the validation doesn't happen automatically for queries and variables, so you have to modify the query first before variables are validated again

@bboure
Copy link
Contributor Author

bboure commented Jul 31, 2023

no, there is a regression in the demos, where someone changed the mode instantiation pattern, so when the schema is fetched, the validation doesn't happen automatically for queries and variables, so you have to modify the query first before variables are validated again

Ah! I noticed this too and drove me crazy. It was next on my list to have a look at and fix.
You'll probably know better than me how to fix, but if you point me in the right direction, I can also have a look

@acao
Copy link
Member

acao commented Aug 2, 2023

@bboure I have some more fixes on the way, as you can see in #3384

do you mind if I merge that first, and you can rebase? or would you prefer the opposite?

I would like to merge this PR sooner than later, because I have some more improvements slated for the monaco mode via variablesJSONSchema for the graphiql monaco effort, and the changes here would conflict heavily with what I have planned

@bboure
Copy link
Contributor Author

bboure commented Aug 2, 2023

@acao I can rebase as soon as you merge the other.

@acao
Copy link
Member

acao commented Aug 2, 2023

ok, in that case, I will just open my PR against this one tonight, because that PR will take a while to get reviews before we can merge

@acao acao force-pushed the fix-variableJSONSchema-optional-input branch from d8703e9 to e4a3d28 Compare August 3, 2023 10:42
@bboure bboure changed the title fix: VariableJSONSchema optional input types feat: Add support for custom scalar validation Aug 6, 2023
@acao
Copy link
Member

acao commented Aug 9, 2023

@bboure ok! this looks good to merge, thanks for all of this!

@acao acao merged commit 7b00774 into graphql:main Aug 9, 2023
12 checks passed
@acao acao mentioned this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants