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

feat(internal): generate proper types for cells with custom props #6014

Merged
merged 13 commits into from
Jul 26, 2022

Conversation

codekrafter
Copy link
Contributor

Ref discord conversation at https://discord.com/channels/679514959968993311/998984413167767623

This modifies cell type generation to try to find a beforeQuery statement. If it finds one, it uses the first parameter's type as the Props type, instead of the GraphQL query variables.

@netlify
Copy link

netlify bot commented Jul 21, 2022

Deploy Preview for redwoodjs-docs ready!

Name Link
🔨 Latest commit 44f5bc5
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62df77f06031080009c71f7f
😎 Deploy Preview https://deploy-preview-6014--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.

@nx-cloud
Copy link

nx-cloud bot commented Jul 21, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 44f5bc5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@peterp
Copy link
Contributor

peterp commented Jul 22, 2022

Very cool!

@noire-munich noire-munich requested a review from dthyresson July 22, 2022 07:48
@noire-munich
Copy link
Collaborator

noire-munich commented Jul 22, 2022

thanks a lot for this @codekrafter , that's a cool addition to cells 👍🏻 .

@dthyresson I'm too fresh at typescript to be confident in my review, I'd approve it but would appreciate a second opinion.

(Edit: posted my approval as I took the opportunity to learn about what puzzled me at first sight ).

@dthyresson
Copy link
Contributor

@dac09 and @Tobbe I was assigned to review this PR, but I'm not sure I follow and this is deep into Cell types. Should we discuss?

@dthyresson
Copy link
Contributor

Thanks @codekrafter for this!

I think we'll need some test of this behavior. Maybe in the test project to make a cell query for e2e? Or better yet in createCell.test?

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

@codekrafter Please see prior comment. Could you add a test in createCell.test to verify this behavior, thanks!

@dthyresson
Copy link
Contributor

@codekrafter after leaving the comment, @Tobbe let me know that it is hard to test a TS change. If that's the case, let me know and perhaps instead we add documentation to showcase the behavior and options in the cell docs?

@codekrafter
Copy link
Contributor Author

@dthyresson

It is a little hard to test just types, but exploring that testing file I was able to add a general beforeQuery test (which was missing before). There was also a small typing issues with the actual createCell implementation that popped up while adding that test I added a fix for.

I also added some content to the beforeQuery section of the Cell documentation to cover this behavior

@dthyresson
Copy link
Contributor

@dthyresson

It is a little hard to test just types, but exploring that testing file I was able to add a general beforeQuery test (which was missing before). There was also a small typing issues with the actual createCell implementation that popped up while adding that test I added a fix for.

Thanks -- I now understand much better.

I also added some content to the beforeQuery section of the Cell documentation to cover this behavior

Amazing, Thanks so much for this!

@dthyresson dthyresson dismissed their stale review July 25, 2022 19:41

Testing behavior isn't possible since this is a TS change, but docs were added to explain instead.

@dthyresson dthyresson requested a review from Tobbe July 25, 2022 19:41
@dthyresson
Copy link
Contributor

@Tobbe LGTM but since you alerted me to the TS change and testing issue, would be great to get your take on the documentation update to explain use.

@codekrafter
Copy link
Contributor Author

Just noticed I had a small typo in the docs for one of my examples, should be fixed now.

@jtoar jtoar enabled auto-merge (squash) July 26, 2022 05:13
@jtoar jtoar merged commit 3b37acc into redwoodjs:main Jul 26, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 26, 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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants