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

Support custom @id column name in scaffold #8264

Merged

Conversation

samthuang
Copy link
Contributor

@samthuang samthuang commented May 9, 2023

An improvement to the scaffold feature (and other generators) allows columns with custom names to serve as the @id field in the Prisma data model. The scaffold feature assumes that the @id field is named "id" by default. This change provides flexibility to specify any name for the @id column if it is marked with the @id attribute in the Prisma data model.

Changes

  • Update the scaffold feature to accept a custom @id column name during code generation.
  • Introduce helper functions to get id name to account for the custom @id column name in the generated code.
  • Update template files: routes, page, cell, component, SDL, and service.
  • Add tests to ensure the scaffold feature functions correctly with custom @id column names.

resolves #5649

@jtoar jtoar added the release:feature This PR introduces a new feature label May 9, 2023
@jtoar jtoar self-assigned this May 10, 2023
@jtoar jtoar self-requested a review May 10, 2023 18:00
@jtoar
Copy link
Contributor

jtoar commented May 12, 2023

Hey @samthuang thanks for taking this one on, this part of the codebase is a little gnarly so it's super appreciated. I tried it locally and there's still some things to work out. I pushed one commit with a fix here: 45ed22b. This got me a little farther in creating a new user with a custom ID. Here's where I'm at now:

image

It looks like the custom ID is being generated as a required field on the create SDL (in this example, my custom ID's name is myId):

   input CreateUserExampleInput {
+    myId: Int!
     email: String!
     name: String
   }

Update: looks like we have to add the custom ID name to IGNORE_FIELDS_FOR_INPUT here:

const IGNORE_FIELDS_FOR_INPUT = ['id', 'createdAt', 'updatedAt']

@jtoar jtoar added the fixture-ok Override the test project fixture check label May 12, 2023
@samthuang
Copy link
Contributor Author

hey @jtoar - sorry for the slow response; I've been on the road for work (actually in SF now). And just to clarify on the next step, we have to somehow find a way to include the custom ID name in that IGNORE_FIELDS_FOR_INPUT array right (conceptually)?

@jtoar
Copy link
Contributor

jtoar commented May 17, 2023

@samthuang yep that's right!

@samthuang
Copy link
Contributor Author

hey hey @jtoar and team - I just want to give a quick update. I've been occupied with other works recently and haven't have the chance to visit this PR, but I still plan to work on this if possible. Hopefully to get some focus time soon!

@samthuang
Copy link
Contributor Author

sorry it's taking me a while to wrap up the PR. I will have a ready-for-review version by the end of this week : )

@samthuang samthuang marked this pull request as ready for review July 6, 2023 08:38
@samthuang
Copy link
Contributor Author

samthuang commented Jul 6, 2023

Hello @jtoar - again, sorry it took a while to prepare the PR for review. Please let me know if this enhancement is still needed. Also, I am unsure why the test is failing : /

Screenshot 2023-07-07 at 17 51 28

@Tobbe
Copy link
Member

Tobbe commented Dec 19, 2023

@samthuang Thanks a lot for this PR. This is impressive work! So sorry we lost track of this PR 🙁

I just tried running the tests locally (thanks for writing those!) and I'm seeing some failures

image

Are you still interested in working on this PR? If so, could you try to see why the tests are failing?
I really like the work you've done here, so I can't promise I won't go and try to fix the tests myself, just to be able to merge this PR 😆

EDIT: Yeah, I couldn't help myself. I went ahead and fixed the tests and merged the PR 😄

@Tobbe Tobbe added this to the next-release milestone Dec 19, 2023
@Tobbe Tobbe merged commit 505c6fc into redwoodjs:main Dec 19, 2023
32 checks passed
@samthuang
Copy link
Contributor Author

@Tobbe, thank you so much for carrying the PR to the end! sorry i have not been active in a while in following up 😅 . just curious, but how did switching to generating TS files resolve the bug?!

@Tobbe
Copy link
Member

Tobbe commented Dec 20, 2023

just curious, but how did switching to generating TS files resolve the bug?!

files in the code below will be a map with file paths as keys, and file contents as values

  let files

  beforeAll(async () => {
    files = await scaffold.files({
      ...getDefaultArgs(defaults),
      typescript: true,
      model: 'CustomIdField',
      tests: true,
      nestScaffoldByModel: true,
    })
  })

At some point after you created this PR we changed to .jsx for JavaScript files with JSX syntax in them. So some filepaths were now like /path/to/SomeCell.jsx instead of /path/to/SomeCell.js, and so the lookup for file contents returned undefined.

So I could have just updated the file paths to .jsx instead of .js. But I wanted to make sure the tests also worked with TS projects as I believe that's what most of our users choose these days.

Tobbe added a commit that referenced this pull request Dec 21, 2023
Co-authored-by: Dominic Saadi <[email protected]>
Co-authored-by: Tobbe Lundberg <[email protected]>
@Tobbe Tobbe modified the milestones: next-release, v6.6.0 Dec 22, 2023
@samthuang
Copy link
Contributor Author

Ah! Got it, thank you so much for taking the time to explain 😄 really appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UUID support for scaffold generation
3 participants