-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Adds option to cell generation for lists of a given model #2569
Conversation
I got to green on tests, including Thus also was unable to test the "list variant" for a destroy just using the name -- I did not add the list option to destroy.
Note: @thedavidprice It might be due to #1814 and just was not noticed yet? I will test tomorrow with the canary release to see if that's the case -- and #1814 introduced this issue and not my changes. |
Note I don't think these changes broke destroy as can see that
And I don't think the destroy tests catch this since they say - get me the generated files and delete them ... and they do but the list of generated files in the test is not the same as when you defacto generate with test, stories, mocks etc. |
packages/cli/src/commands/generate/cell/__tests__/__snapshots__/cell.test.js.snap
Outdated
Show resolved
Hide resolved
cellName = options.list | ||
templateNameSuffix = 'List' | ||
} else { | ||
// needed for the singular cell GQL query find by id case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note pretty, but needed for the destroy tests because for some reason they could never find the query engine binary.
Thanks for the very detailed PR description 👍 Not sure I understand this correctly, but is it true that in So I haven't had much luck with my naming suggestions either so far, but that doesn't mean I'll stop trying 😅
For the last two, special care needs to be taken to make sure |
Yes, you are correct. FYI, you don't need the So,
is the current use. As I noted in a comment, I'm not fully taken with the naming either my favorite currently alternative is:
or
Again, the main issue is that prior to this PR, the cell generator when told to make a
generates a cell with GQL note the
for the export const users = () => {
return db.user.findMany()
} and is wrong. And that doesn't make sense either. If your export const user = ({ id }) => {
return db.user.findUnique({
where: { id },
})
} that it needs. So, this PR sorts that out and maybe The other approach would be to:
That might be more friendly .... and then |
In those three, I just find it weird that the first argument after
(Personally I get confused by Or, as I also proposed above, which I think reads better:
or
|
It looks up the model -- or rather it references/matches the Model in your schema. |
Lets pair on this DT when you're back on - have some ideas. Thank you for helping me out! |
I think this would be very difficult to implement due to the way generators consider positional vs optional params -- but maybe it could work. |
Co-authored-by: Daniel Choudhury <dac09@users.noreply.github.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
* 'dt-plural-cell-names' of github.com:dthyresson/redwood: Update packages/cli/src/commands/generate/helpers.js Refactored plural cell generator Get to green on cell destroy Update snapshots/ schema issue. Destroy cell fails Fixes unordered list success rendering Renders an unordered list Adds list option to cell generation
packages/cli/src/commands/generate/cell/templates/cellList.js.template
Outdated
Show resolved
Hide resolved
Could I get an approval? We paired on this technically, so I'm giving my implicit approval :D |
That sounds a lot more straightforward; the |
@dac09 I feel like the DX around this hasn't been nailed down yet? But if we can discuss that later then I can give approval; feels like we need some more input from @mojombo and/or @cannikin on the naming of things especially. But this is definitely an improvement and the new templates are great; the ones before were definitely not super intuitive. |
While thats still valid, it doesn't solve our list vs findUnique problem. i.e. when I generate a cell for equipment, how do you know if i meant cell for an equipment by Id or a cell to get a list of all equipment. Detecting plural, like in the current solution, does! |
@dac09 Totally; and more than ok with the flag in that case, but if things can be pluralized, can the flag be skipped? I.e. the user/users case, will things just work since user can be pluralized? |
yep, if we can detect that you passed in plural, it will just work. You only really need to pass the flag in if: |
…-codegen * 'main' of github.com:redwoodjs/redwood: Adds option to cell generation for lists of a given model (redwoodjs#2569) Router: More helpful typings for Route (redwoodjs#2592) upgrade Prisma v2.23.0 (redwoodjs#2566) add crowdin contribs (redwoodjs#2560) `rw exec` to run arbitary scripts in Redwood context (redwoodjs#2548)
Woah, way to go @dthyresson 🚀 Curious where things are at with Destroy cleaning up tests, stories, and mocks. If you remember, please update at the next meeting (if applicable). |
This PR fixes issue #2565
This PR updates:
ul
with itemsmanyEquipment
)Given the model
User
and normal general modelGenerates:
And the mock:
Given the model
User
and list option, or pluralising usersor just
And the mock:
Note: Seeing why the unique Operation name is
UsersQuery_2
-- it may think that query already exists.Detecting plurals
When we can detect that the word passed is in plural, we generate list cells.
e.g.
Will generate a query for a list of members
Handling difficult words
When we can't detect whether the passed in word is plural or not, we assume its singular.
e.g.
Will generate a query for
FindEquipment
(by id)but
Will generate a query for a list of equipment