Scaffolds: Don't generate unused methods

[Tobbe]

When using the scaffold generator you often end up with unused methods like this

This PR looks at what methods will be needed, and only includes those that are used.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b29f87d. 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

---

🟥 Failed Commands
cli run test --concurrency 2 -- --colors --maxWorkers 2

✅ Successfully ran 8 targets

• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build

---

Sent with 💌 from NxCloud.

[netlify]

✅ Deploy Preview for redwoodjs-docs ready!

Name	Link
🔨 Latest commit	b29f87d
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62fcf7766902c800090650c5
😎 Deploy Preview	https://deploy-preview-6221--redwoodjs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[cannikin]

I always thought it would be good to have those so that if/when you expand on the model that's backing that scaffold, you have those functions ready to go. Otherwise you need to write your own or find a lib on NPM.

We could move them to an internal lib and import them at the top (maybe something like @redwoodjs/web/scaffold). And if we determine you don't need them, we comment out the import with an additional comment above about what you may want to use them for?

[Tobbe]

Here are some counter points 🙂

I think the chances that you're going to need/want those exact implementations are rather slim.

When you update your underlaying model you either re-run the scaffold generator, or you know you're going to have to update a whole lot of code in a whole lot of places. Including writing whatever custom display method you might need.

And also, especially for a super basic one like jsonDisplay it's more mental overhead for me to have to learn about the existence of that method, and how to use it, than to just write the small utility function myself when/if I need it.

[Tobbe]

From @dac09:

I think having as little logic like this in the template is a good idea in general. As we discussed before - it might just make a lot of sense instead to put these functions into a lib/utils file.
It also reduces duplication of these utility functions
I would prefer [to put the functions in the] user’s project

(Content in square brackets added by me for context)

[jtoar]

I don't mind exploring the @redwoodjs/web/scaffold idea. But right now I'm leaning towards if they're not needed, let's leave them out (or comment them out—anything to get rid of the red squiggles). I definitely like teaching users about Redwood's API surface where we can, but the community seems to be settling on the opinion that a learning opportunity isn't worth the squiggles. (Honestly I still don't really mind using the ignore-unused-variables ESLint rule, but we've already hashed that one out a bit and I don't want to make us bikeshed too much)

[virtuoushub]

In a more general sense, I have been wanting to make the generators more composable so that we have a set of defaults and then you can use something like CLI args to turn on or off certain parts of the code-gen at a much more fine grained level then we currently do ( e.g. Add ability for stories around "stock" Redwood components (e.g. NotFoundPage, SignupPage, LoginPage, ...) to be generated #5269 ).

Not sure if/how it fits in here, so in the immediate I am in favor of leaving the unused methods out.

[dthyresson]

I always thought it would be good to have those so that if/when you expand on the model that's backing that scaffold, you have those functions ready to go. Otherwise you need to write your own or find a lib on NPM.

I think that most people who add a field to a model -- and use scaffolds -- are most likely to regenerate the scaffold than to edit the components manually to add the field in the form ... and if they didn't they would have to know how use use the util functions. Yes, they may see them ... but there's no guarantee they will use them when manually adding the field. And they will remain unused.

Alternatively, if they regenerate the scaffold, then the util function knows it is needed and then is applied.

I think that leaving them out delivers initially cleaner code -- but the utils could be in the user's app so they can reference them later ... or replace with other validators/formatters.

If fact, I anticipate that when certain urls can be shared across web/api a little easier, then these formatters and validations will be that shared url that both the api and web form sides could use.

[Tobbe]

So I implemented the idea of having a separate file for all these display methods. A few observations

• The template files are much simpler. Less code and less logic.
• It had to be a .tsx file because some of the functions return JSX. So no sharing between sides
• In a real-world app, do we really think the display functions will be generic enough to share between different model lists? Or will everyone end up writing custom display functions inside their model list components anyway?
• If someone does edit one of the display functions it might break future scaffolded models.

(I didn't bother updating any of the tests. Didn't want to spend time on it until we've decided what to do here)

[Tobbe]

rob: put them in a separate file
tobbe: leave them out
dom: leave them out
pc: leave them out
thomas: put them in a separate file (talked to him on Slack)
danny: put them in a separate file

@dthyresson I'm not sure I know what way you're leaning currently. Did my latest comments above change your reasoning at all?

[dac09]

For reference, and I'm not necessarily saying it has to be this way, I have it currently in a web/src/utils folder - but I think lib is better because consistency with the api side.

I'd probably just call it formatters.tsx though, so it reads like

import { formatEnum } from 'src/lib/formatters'

I also (personally) like putting extensions in my file names for this stuff, not sure if there's any appetite for it - so for example

• display.formatters.tsx
• copy.helpers.ts
• share.helpers.ts

[dthyresson]

@Tobbe My take is that if we leave them in (a separate file) or leave out -- we will have to document both cases.

If the are in a file, the docs will say - if you manually add an Enum or a DateTime, then "use these helpers".

If they are not in the file, then the docs will have to suggest a way to format them -- or ask the developer to come up with his/her own method.

Given those two options, I now think that they should be in a separate file that can be easily documented in the Scaffolding or Form docs to point to each method and what it does and when to use.

[dac09]

Bumping this - shouldn't fall of our radar. @cannikin - you had another PR with scaffold changes. Worth coordinating the two!

[cannikin]

Yeah, mine is ready to go #6385 but I thought we could review in the next core meeting to see if it's worth it. I'd really like to get @mojombo's opinion of having all the text/labels for the scaffold inside the route itself...we've been trying to keep that as concise and readable as possible, but this maybe starts mixing concerns a little bit (routing and content).

[dac09]

Isn't this dependency going to be a problem? I know it probably was there before, just not something I hadn't seen.

It looks like its a dependency on cli, but not web, so we might get away with it.

[Tobbe]

Since the import is explicitly in the users's project, the dependency should probably also be explicit in the project's package.json, right? Should I make the scaffold generator run yarn add?

[dac09]

This is a weird one... maybe we should add it as a dependency on rwjs/web? It feels like a hammer, but atleast its "backwards compatible"

The reason it may not be an issue is because webpack will bundle dependencies anyway. So the other option is to not do anything, since its part of the cli dependencies anyway.

[dac09]

using variable names here would be super helpful :)

[Tobbe]

ugh. 100% agree!