Check for gql documents before running codegen

[Tobbe]

Fixes #4393

Recap

This is the error we're printing

gen | Generating TypeScript definitions and GraphQL schemas...
gen |
gen |       Unable to find any GraphQL type definitions for the following pointers:
gen |
gen |           - ./web/src/**/!(*.d).{ts,tsx,js,jsx}
gen |
gen |
gen |       Unable to find any GraphQL type definitions for the following pointers:
gen |
gen |           - ./web/src/**/!(*.d).{ts,tsx,js,jsx}
gen |
gen |
gen | Error: Could not generate GraphQL type definitions (web)
gen |
gen | 14 files generated

The "Unable to find..." error message comes from here: https://github.com/ardatan/graphql-tools/blob/2207fbcdacf708ee9836ee72d0c0f92dba0d0942/packages/load/src/load-typedefs.ts#L120

The "Error: Could not generate..." message is our own.

Fix

This PR uses the loadDocuments() function from @graphql-tools to first try and load all gql documents. If no documents are found we don't run the codegen, and thus don't get the error messages shown above.

In a recent meeting with The Guild there was some talk about performance issues. Especially when reading from disk as we're doing here. So I did some rudimentary timing checks on this. From what I can tell this extra check adds less than 0.1s to the total runtime for yarn rw g types.

This is an alternative fix to #4491. That PR didn't do anything about the running of the codegen, it only changed the "Error: ..." message.

Before

After

[Tobbe]

@dac09 Just for you 🙂

Check for gql documents before running codegen by Tobbe · Pull Request #4728 · redwoodjs/redwood - Vivaldi

[dac09]

Requesting clarification and minor changes, comments in the files

[Tobbe]

@dac09 Thanks for your review. If I understood you correctly the biggest concern was that we might be catching too many errors when calling loadDocuments. I tried to write some tests for that. I'm not super happy with the tests because 1) I had to introduce a function param only used for tests, and 2) I'm testing for strings inside error messages, which can make the tests unnecessarily brittle. But I do at least think they cover the concerns you had.

[dac09]

1. I had to introduce a function param only used for tests, and But I do at least think they cover the concerns you had

Yes they do for sure. I think its worth the compromise, I'm sure you would've done it another way if possible!

2. I'm testing for strings inside error messages, which can make the tests unnecessarily brittle

Maybe switch this to use inline snapshots? May not address the brittle problem, but will be much easier to maintain!

Something like:

console.error = jest.fn() 

expect(console.error.mock.lastCall).toMatchInlineSnapshot()

Watcha think?

[dac09]

Another thing to check based on our conversation yesterday:

• Can we catch the specific error for when a model is missing? (i.e. relations) when we loadSchema

[thedavidprice]

#want this is v1 so so bad 🥺🙏

[Tobbe]

I'll do my best

[thedavidprice]

I'll do my best

Thanks @Tobbe I know you are and you will. Excited to have this far along!!

[netlify]

✅ Deploy Preview for redwoodjs-docs ready!

🔨 Explore the source changes: 793dd10

🔍 Inspect the deploy log: https://app.netlify.com/sites/redwoodjs-docs/deploys/6238f40eb47b6400096c1109

😎 Browse the preview: https://deploy-preview-4728--redwoodjs-docs.netlify.app

[Tobbe]

Another thing to check based on our conversation yesterday:

• Can we catch the specific error for when a model is missing? (i.e. relations) when we loadSchema

This would be inside generateGraphQLSchema. I didn't plan to touch that file in this PR

[Tobbe]

I couldn't figure out how to fix this test error:

Why is it generating snapshots inside /dist? And why is this working locally, but not here?

Anyway, I stoped using snapshot tests (for now). If anyone knows how to make them work, please let me know. Otherwise I'm good with just leaving the tests as they are now.

[Tobbe]

@dac09 Ready for another review

• I moved all the graphql codegen stuff out of typeDefinitions.ts into graphqlCodeGen.ts. Same for tests.
• It now uses codegen from @graphql-codegen/core instead of generate from @graphql-codegen/cli
• I've only done this for graphql documents. I have not touched graphqlSchema.ts. I could switch that one over as well, but would prefer to do so in a separate PR.
• I'm not using any snapshot tests. For some reason jest was putting snapshots in both src/__tests__/__snapshots__ and dist/__tests__/__snapshots__ (and some other test files too in /dist). This made the testcases fail on Github.
• Users can supply their own codegen.yml that will be merged with RW's internal codegen config. That file is mainly for codegen/cli. Because of that the users don't have as much control over the codegen process. Only root level config is merged. I think I can improve this a little bit if need, but likely not make it as powerful as it was previously. The original testcase that was written for this still passes.

[dac09]

Minor suggestion. what do you think of mocking fs.writeFileSync here instead of using the index here?

[Tobbe]

I really like this idea. Saves on disk I/O for the test case which is always a good idea.

[dac09]

This is awesome! Thank you @Tobbe for bearing with me.

Looks like it should be possible to catch the "unknown type" error too, which is awesome. And no problem doing this in another PR

Left a few small comments that I would appreciate if you addressed

[dac09]

Do you think we could:
a) Move this into a separate describe block
b) Before each, delete all generated .d.ts files, to make sure the old ts files in the fixture aren't picked up
c) Make sure the generate .d.ts isn't commited in the missingType fixture

[Tobbe]

a) done
b) I've (manually) deleted all .d.ts files. No "old" files exist
c) No files are generated as part of the tests, because the generation bails on the (expected) errors. If, for some reason, files starts being generated hopefully that's a red flag for the developer so he/she doesn't just blindly commit them...