fix: make codegen more robust #311
Conversation
src/cmds/codegen/codegen.ts
Outdated
| @@ -128,27 +127,41 @@ export class Codegen { | |||
| } | |||
|
|
|||
| if (generator === 'typegen') { | |||
|
|
|||
| if (!output.typings || output.typings === '') { | |||
| throw new Error("Please provide output.typings path to use typegen") | |||
There was a problem hiding this comment.
Can we make this error message actionable? How can I usually fix this problem?
There was a problem hiding this comment.
Please provide output.typings path in graphql config to use typegen - does this sound better? I basically want to notify the user that they need to provide the typings option in the codegen extension.
src/cmds/codegen/codegen.ts
Outdated
|
|
||
| if (child.error) { | ||
| if (child.error.message === `spawnSync apollo-codegen ENOENT`) { | ||
| throw new Error(`Generator apollo-codegen is not installed.`) |
There was a problem hiding this comment.
Can we make this error message actionable? How can I usually fix this problem?
There was a problem hiding this comment.
Ideally, we will not hit this at all with this new code!
src/cmds/codegen/codegen.ts
Outdated
| @@ -158,16 +171,31 @@ export class Codegen { | |||
| // fs.unlinkSync(tmpSchemaPath) | |||
| } else { | |||
| const args = ['--input', inputSchemaPath, '--language', language] | |||
|
|
|||
| if (!output.binding || output.binding === '' && !output.typeDefs || output.typeDefs === '') { | |||
| throw new Error("Please provide either output.binding or output.typeDefs to use this generator") | |||
There was a problem hiding this comment.
Can we make this error message actionable? How can I usually fix this problem?
There was a problem hiding this comment.
Again, I want to notify that use that in codegen extension, they need to provide either binding or typeDefs option to use this generator.
There was a problem hiding this comment.
Ok, let's add the reference to GraphQL Config again 🙂
|
Changes in terms of error message are done as suggested by @marktani Testing on windows is in progress. |
|
Not ready for windows yet, one of our Pondering a solution. |
npm-run 5.x is broken on windows we don't want to accidently merge it unless timoxley/npm-run#19 is resolved.
|
Current solution is to downgrade This is ready to be merged now. Please let me know if any changes are needed. Thanks! |
|
🎉 This PR is included in version 2.16.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Great job tracking this down, any chance to also downgrade npm run for prisma itself to make sure |
|
@ntziolis : I am trying to replicate the |
This PR does the following:-
Fail fast on any errors and notify the user about a potential fix
Refactor the logic to locate
apollo-codegenbinary to supporttypegenProvide a default input value to
typegenDowngrade
npm-runto get around the regression on windowsI am testing this one one windows after which we can merge this and hopefully make our codegen users happy. Thanks!