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

Tutorial 6.3: Fix prisma type error on comment creation #6045

Merged
merged 3 commits into from
Aug 13, 2022

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Jul 26, 2022

Using postId in the input for the createComment-test in api/src/services/comments/comments.test.ts gives the following red squiggle:

Type '{ name: string; body: string; postId: number; }' is not assignable to type 'CommentCreateInput'.
  Object literal may only specify known properties, but 'postId' does not exist in type 'CommentCreateInput'. Did you mean to write 'post'?ts(2322)
comments.ts(22, 3): The expected type comes from property 'input' which is declared here on type 'CreateCommentArgs'

Solutions up for debate:

  1. use Prisma.CommentUncheckedCreateInput instead of Prisma.CommentCreateInput, thus being able to reuse the current JS test code on the TS tab solution without getting a type warning. This is what 53dfb68 does
  2. Don't use postId, but rather connect the comment to the existing post using Prisma logic for both JS and TS code:
      const comment = await createComment({
        input: {
          name: 'Billy Bob',
          body: 'What is your favorite tree bark?',
          post: { connect: { id: scenario.post.bark.id } },     👈🏻
        },
      })

I had just commited the approach for 1. when i remembered the connect syntax from Prisma. I'm unsure about the implications (which is faster, what is being done under the hood, are there additional checks), but as the only alternative to using the connect-syntax is using the "unchecked" input – that may be a hint that the "proper" / preferred Prisma-way of doing this would be the approach 2.

Looking forward to hear other people's thoughts on this.

@nx-cloud
Copy link

nx-cloud bot commented Jul 26, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 2dd44c6.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jul 26, 2022

Deploy Preview for redwoodjs-docs ready!

Name Link
🔨 Latest commit 2dd44c6
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62f7d918ec46190009088c0b
😎 Deploy Preview https://deploy-preview-6045--redwoodjs-docs.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 settings.

@dac09
Copy link
Contributor

dac09 commented Jul 28, 2022

@Philzen are you using strict mode in TS?

Thanks for the PR!

@Philzen
Copy link
Contributor Author

Philzen commented Jul 28, 2022

are you using strict mode in TS?

Not that i would know of. 😉
When i annotate something as any, I'm getting a yellow squiggle warning about a violation of rules/no-explicit-any, so i believe that is a clear indicator strict mode, isn't it? (still in a daily struggle to wrap my head around TS, but that's what i understood so far about strict mode).

@Philzen Philzen marked this pull request as draft July 28, 2022 12:07
@Philzen
Copy link
Contributor Author

Philzen commented Jul 28, 2022

BTW I've put this as draft for now until we a consensus which solution is preferred.

I feel both approaches have something to it, i'm slightly leaning towards No. 2 currently, where we could briefly explain the approach, adding a link to the connect syntax doc – but also add an info box explaining how the unchecked variant works if people prefer to use postId directly. Or vice versa. Best of both worlds, with maximum learning effect (and maximum tutorial size growth 😅)

@dac09 dac09 added the release:docs This PR only updates docs label Jul 28, 2022
@Philzen
Copy link
Contributor Author

Philzen commented Aug 7, 2022

@dac09 Just wanted to quickly check back if there has been any response so far from the core team on which approach is preferred?

@Tobbe
Copy link
Member

Tobbe commented Aug 7, 2022

I prefer the "connect" version. I think it more clearly shows the intent.

Found this Prisma issue that also seems to favor "connect" prisma/prisma#4322

For now you will need to either choose use one of the syntaxes. My recommendation would be to if you fallback to connect method if you need connect via some other field.

uncheckedScalar is just a shortcut for some very common use cases. You can always resort to old style if you have multiple or nested values.

If we teach people about the "connect" way of doing it, they will be able to handle all future relation needs they might have. If we only teach them about the "unchecked" way, they will have to discover and learn the "connect" way themselves when they run into the limitations of the unchecked way.

Here's a relevant StackOverflow answer https://stackoverflow.com/a/69169106/88106

@Philzen Philzen force-pushed the patch-14 branch 3 times, most recently from 705a66e to 855643d Compare August 9, 2022 00:38
@Philzen Philzen marked this pull request as ready for review August 9, 2022 00:38
@Philzen
Copy link
Contributor Author

Philzen commented Aug 9, 2022

I prefer the "connect" version. I think it more clearly shows the intent.

An excellent choice, Sir @Tobbe 🧐

😉

I've updated the PR and also added a separate to commit, trying myself on an entertaining info-box that explains what we're looking at there (and what the alternatives are, also regarding TypeScript definitions).

Not sure how good it is. May be worthwhile having some additional information in there. But by no means this is the wittiest, funniest infobox people will have ever read in the tutorial, so i'm open for any changes or even dropping it, it's merely a suggestion / shot from the hip.

@Philzen Philzen force-pushed the patch-14 branch 2 times, most recently from 840671d to 9e0ac91 Compare August 10, 2022 13:54
Philzen and others added 2 commits August 10, 2022 16:13
…-test

Otherwise a typescript error is generated when trying to provide
an input that directly contains a postId
@Tobbe Tobbe enabled auto-merge (squash) August 13, 2022 17:02
@Tobbe Tobbe merged commit 55e49d6 into redwoodjs:main Aug 13, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Aug 13, 2022
@jtoar jtoar modified the milestones: next-release, v3.0.0 Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:docs This PR only updates docs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants