CODEGEN-334 - Throw custom NoTypeDefinitionsFound if no document found whilst loading documents #7093
Conversation
…ing for libraries to handle
🦋 Changeset detectedLatest commit: 809bc08 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces a new error handling mechanism in the GraphQL tools with the Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as GraphQLLoader
participant PR as prepareResult
participant Caller as Test/Client
Caller->>Loader: Request type definitions load
Loader->>PR: Invoke prepareResult with pointers
alt Valid type definitions found
PR-->>Loader: Return type definitions data
Loader-->>Caller: Deliver type definitions data
else No type definitions found
PR-->>Loader: Throw NoTypeDefinitionsFound
Loader-->>Caller: Propagate NoTypeDefinitionsFound
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/load/src/load-typedefs.tsOops! Something went wrong! :( ESLint: 9.24.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| '@graphql-tools/load': minor | |||
There was a problem hiding this comment.
This feels like a minor, rather than a patch because it's not a bug per se
It's could be a major if consumers specifically check Error case 🤔
Happy to be swayed either way here
There was a problem hiding this comment.
It is still Error right? So it should be good imo.
| await load(glob, { | ||
| loaders: [new GraphQLFileLoader()], | ||
| }); | ||
| expect(true).toBeFalsy(); |
There was a problem hiding this comment.
Not only related to this test but I found expect(true).toBeFalsy(); will actually throw an error, which will satisfy the expect(e).toBeDefined(); statement below
i.e. the tests in this file will always pass 😅
packages/load/src/load-typedefs.ts
Outdated
| @@ -135,7 +135,7 @@ function prepareResult({ | |||
| const pointerList = Object.keys(pointerOptionMap); | |||
|
|
|||
| if (pointerList.length > 0 && validSources.length === 0) { | |||
| throw new Error(` | |||
| throw new NoDocumentFoundError(` | |||
| Unable to find any GraphQL type definitions for the following pointers: | |||
There was a problem hiding this comment.
Let's move this message inside the new error then. And the constructor can take pointerList.
There was a problem hiding this comment.
Nice, that makes sense. It's done here: 95258f5#diff-861d37e0c7d8a1469880c96a03a643fdb722c21b7304a4324a42c85c59e7ef8dL138-L144
There were some options I also considered
e.g. throw new NoTypeDefinitionsFound('No type defintions found', { pointerList }). However, the message is just a duplicate.
So passing pointerList as the first param feels better, even though most Error's first param is a message
- Rename custom error to NoTypeDefinitionsFound to match the rest of implementation - Let custom error take pointerList and encapsulate error message logic - Update tests to handle sync vs async use cases - Concat rows instead of using template literal strings to have better formatting
| @@ -151,3 +145,17 @@ function prepareResult({ | |||
| debugTimerEnd('@graphql-tools/load: prepareResult'); | |||
| return sortedResult; | |||
| } | |||
|
|
|||
| export class NoTypeDefinitionsFound extends Error { | |||
There was a problem hiding this comment.
Note: I've changed this from NoDocumentFoundError to NoTypeDefinitionsFound to better match other names in this file
packages/load/src/load-typedefs.ts
Outdated
| const rows: string[] = []; | ||
| rows.push('Unable to find any GraphQL type definitions for the following pointers:'); | ||
| pointerList.forEach(pointer => { | ||
| rows.push(`- ${pointer}`); | ||
| }); | ||
| const message = rows.join('\n'); |
There was a problem hiding this comment.
I'm joining strings this way because I find the template literal adds a lot of extra spacing on the left of the terminal.
| @@ -93,14 +93,22 @@ describe('documentsFromGlob', () => { | |||
| }); | |||
|
|
|||
| test(`Should throw on empty files and empty result`, async () => { | |||
| expect.assertions(2); | |||
There was a problem hiding this comment.
I've switched back to try/catch with an expect.assertions(2) and no expect in the happy path. This helps us fail the test if load somehow succeeds
| @@ -135,13 +135,7 @@ function prepareResult({ | |||
| const pointerList = Object.keys(pointerOptionMap); | |||
|
|
|||
| if (pointerList.length > 0 && validSources.length === 0) { | |||
| throw new Error(` | |||
| Unable to find any GraphQL type definitions for the following pointers: | |||
There was a problem hiding this comment.
It seems we lose the indentation here?
There was a problem hiding this comment.
I've left a note about formatting here: #7093 (comment)
I think it could look better if it doesn't have the indentation & left-aligned. Otherwise, when it'd have some big spaces if an error happens in Codegen:
This is personal preference of course, I'll revert this to preserve the existing behaviour 🙂
eddeee888
changed the title
Description
GraphQL Code Generator uses
@graphql-tool/loadto load documents.When there are no documents found given a pattern,
@graphql-tool/loadthrows an error. However, Codegen doesn't know if it's a syntax error, no document found error or anything else.This PR adds and exports a custom error
NoDocumentFoundErrorto@graphql-tool/loadto allow libraries like Codegen to handle No Document Found error case more explicitlyRelated Codegen issue dotansimha/graphql-code-generator#9172
Type of change
Please delete options that are not relevant.
expected)
How Has This Been Tested?