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

Generate CRUD by default for SDL and Services to mitigate issues in GraphQL used in cells #4785

Merged
merged 10 commits into from Mar 18, 2022

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Mar 17, 2022

Fixes issue: #4766

As noted in the above issue:

When generating first the SDL for a model, and then a Cell for the same model you'll get an error printed to the console, and the generated code will be wrong.

The issue is that the SDL generator doesn't generate the userExample query that the Cell needs. You can "fix"/work around that by running the SDL generator with the --crud flag. The SDL generator by default only generates the "find many" query. I think it should generate the "find unique" query too. The "write prisma model" -> "generate sdl" -> "generate cell" workflow is something we should support by default, without any extra flags.

This PR updates the sdl and service generator to always create CRUD artifacts.

Note:

  • Docs need update to reflect new default (todo)
  • Will tutorial need change in example code output (@cannikin)

@netlify
Copy link

netlify bot commented Mar 17, 2022

✅ Deploy Preview for redwoodjs-docs ready!

🔨 Explore the source changes: ba13525

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

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

@dthyresson
Copy link
Contributor Author

Oh @jtoar I just realized I can now update the docs in this same repo -- and part of this PR! Amazing.

@cannikin
Copy link
Member

To stay consistent (not passing the flag is the same as true) we may want to update the scenarios generator to also not pass the flag when it generates the SDL portion.

@dthyresson
Copy link
Contributor Author

dthyresson commented Mar 17, 2022

To stay consistent (not passing the flag is the same as true) we may want to update the scenarios generator to also not pass the flag when it generates the SDL portion.

@cannikin

I don't think there is a dedicated scenarios option on the api side.

On the web side there are tests and stories.

But, on the api side, the tests use scenarios so I think it's all or nothing.

I'll check again (but that was my impression when making the change).

@dthyresson
Copy link
Contributor Author

I was just thinking remove these two lines since crud is default true now:

https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/scaffold/scaffold.js#L161 https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/scaffold/scaffold.js#L167

Oh, I see in scaffold. Ok that makes sense. Thanks!

@dthyresson
Copy link
Contributor Author

dthyresson commented Mar 17, 2022

I was just thinking remove these two lines since crud is default true now:
https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/scaffold/scaffold.js#L161 https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/scaffold/scaffold.js#L167

Oh, I see in scaffold. Ok that makes sense. Thanks!

I updated https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/scaffold/scaffold.js#L161 for the SDL default in scaffold.

But - the one in services since it passes along in ...rest I didn't want to mess with the logic there ... removing it causes tests to fail. Figured not worth risk to change the rules there for moment,

@dthyresson dthyresson requested review from cannikin and removed request for thedavidprice March 17, 2022 16:18
Copy link
Member

@Tobbe Tobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the --no-crud syntax. I think it's easier to read (and shorter to type) than --crud=false. A quick search through the docs shows we use both variants for other commands, but the --no- version is more common.

docs/docs/cli-commands.md Outdated Show resolved Hide resolved
docs/docs/cli-commands.md Outdated Show resolved Hide resolved
docs/docs/cli-commands.md Outdated Show resolved Hide resolved
dthyresson and others added 3 commits March 18, 2022 15:01
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@dthyresson
Copy link
Contributor Author

I prefer the --no-crud syntax. I think it's easier to read (and shorter to type) than --crud=false. A quick search through the docs shows we use both variants for other commands, but the --no- version is more common.

Thanks! I had forgotten about the “—no” option. That’s much better.

@thedavidprice
Copy link
Contributor

@cannikin I'll get this in when CI passes. Will the tutorial need updating?

@thedavidprice thedavidprice merged commit 1d2e0eb into redwoodjs:main Mar 18, 2022
@jtoar jtoar added this to the next-release milestone Mar 18, 2022
@cannikin
Copy link
Member

Yep: https://redwoodjs.com/docs/tutorial/saving-data#create-an-sdl--service I actually make a point of saying we're NOT using the --crud flag so that we can teach you how to do it manually first.

Copy link
Contributor

Understood. Will you still keep that flow then and just start with --no-crud?

@cannikin
Copy link
Member

Maybe...I tend to want to teach the plain, basic forms of commands and then mention specialty flags in a little callout... I can reword, it's fine. Just makes it look like Redwood does even more stuff for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

None yet

5 participants