[scaffold generator] don't write into existing directory without --force; don't add model-name dir if custom path is passed

thedavidprice commented 3 years ago

Issue 1: Scaffold writing into an existing directory

Given an existing component directory like TeamMember/, if I create a new prisma.schema model of the same name and then run yarn rw g scaffold TeamMember, the generator will write CRUD components into the existing TeamMember/ directory. _This causes import havoc and hard to diagnose bugs. The same can happen with Pages and Layouts.

Proposed solution:

Scaffold generator should check if directories already exist (before writing any files), and then fail with a helpful message if true
option to write into existing directories passing --force
otherwise, devs can either change existing directory or model names or simply use <path>/<model> option for scaffolding

Issue 2: Scaffold generator creates duplicate directory when passing `<path>/<model>`

Using the new nested directory feature for Scaffolds, if I generate for a model TeamMember, e.g. yarn rw g scaffold TeamMember, I will get a directory structure for components, pages, layouts that looks like:

TeamMember/component-dir/component.js

Using `<path>/<model>`

I have the option to use <path>/<model> as well, e.g. yarn rw g scaffold admin/TeamMember. Here is what I expected my directory structure to be:

Admin/component-dir/component.js

However, here is what the actual directory structure is:

Admin/TeamMember/component-dir/component.js

The generator keeps the original nested directory structure and adds the path instead of using the path to override the directory structure. If I had wanted to do that, I would have run yarn rw g scaffold admin/TeamMember/TeamMember

Discussion

I would like to know if my expectations are also general expectations from others. If so, would we agree that the current behavior needs to be changed?

cjreimer commented 3 years ago

Hey David, great points of discussion!

Regarding Issue 1: Ideally, I'd love to see a warning with a prompt to continue if the model-named directory lines up with an existing directory. Do we have a warning / prompt and response system built into any of the redwood CLI commands?

Regarding Issue 2: You can blame me for this one! That being said, I think there are cases where someone could expect either behavior. While there is an option to have redwood perform the configuration you wished, it would currently require adding the following temporarily to your redwood.toml:

[generate]
  nestScaffoldByModel= false

One option could be to add a CLI flag to choose between the two scenarios as to whether you still want the model name in the path or not. Also, once again, a CLI prompt with user response could be useful as well, where we confirm with the user where redwood intends to drop the component files before we actually write them, would be useful.

Thoughts?

thedavidprice commented 3 years ago

Hi @cjreimer! For interactive CLI, we are using Prompts in a few places. It's working great! The main thing with any interactivity is to make sure it won't be something that blocks us during CI. For either of these cases, I don't think it would be an issue.

Regarding Issue 1: Ideally, I'd love to see a warning with a prompt to continue if the model-named directory lines up with an existing directory. Do we have a warning / prompt and response system built into any of the redwood CLI commands?

^^ I think this would be great!

Regarding Issue 2: No blame needed! We're figuring this out as we go. And the work you've done to date is a huge improvement.

As is, here's the behavior I think would be ideal. But needs discussion. For a model named post, the generator:

...default will always be nested, e.g. post/post-directories/code
...when admin/post is passed (e.g. <path>/<model> option), the path will override the default, e.g. admin/post-directories/code
...we would use Prompts to confirm what's about to happen in the case the user passes <path>/<model>

?

cjreimer commented 3 years ago

Hey David!

Regarding item 1, I'm on board with using Prompts! If there ever would be a CI issue on any command, I would simply suggest using the --force flag to override, or adding a custom flag to pre-select the desired option

Regarding issue 2, can you clarify what the difference is between your options 2 and 3? Is there a typo in what you intended, or am I misunderstanding as there appears to be overlap?

I will defer to the core team, but my preference is consistent behavior and I believe that if yarn rw g scaffold model generates the directories in components/model/* then yarn rw g scaffold admin/model should generate the directories in components/admin/model/*

However, let me propose an alternative. I think part of the challenge we are facing is that we have combined the path and model into a single argument in the CLI command. What if we were to change the CLI command and separate out the path argument? (Note: The docs indicate the g page command should work this way but doesn't as per my test on v0.35.2.)

yarn rw g scaffold model [path] [other flags]

This way it can be clear in the documentation that the default path = model, and

yarn rw g scaffold model generates the directories in components/model/*

and yarn rw g scaffold model admin should generate the directories in components/admin/*

and alternatively, yarn rw g scaffold model admin/model should generate the directories in components/admin/model/*

I believe this would result in more consistent expected behavior. Thoughts?

thedavidprice commented 3 years ago

Regarding issue 2, can you clarify what the difference is between your options 2 and 3? Is there a typo in what you intended, or am I misunderstanding as there appears to be overlap?

^^ For 1, 2, and 3, I am attempting to state the behavior I think is preferred — intending them all to be compatible. The idea for number 3 is that if some does specify the directory structure by passing <path>/<model>, then we would first display the path that's about to be used and then prompt them to confirm that's the path they want before proceeding to write. If someone doesn't specify the <path>, then I didn't think the prompt would be necessary. But maybe it's a good idea to show the structure and confirm always.

Is that more clear?

... yarn rw g scaffold admin/model should generate the directories in components/admin/model/*

^^ Using this example, one problem I have with this behavior is that there's no way a user can not have the **/model/* directory. If a user is already specifying the path that's required, then we should assume they would not want this directory unless specified.

(Note: The docs indicate the g page command should work this way but doesn't as per my test on v0.35.2.)

^^ Saw that. Definitely needs fixed as well!

yarn rw g scaffold model [path] [other flags]

+1 to this! Definitely more clear. And the behavior you described feels much more clear.

However, we should first confirm that Yargs will allow us to build out Positionals in order like that (i.e. the order of model and path will matter). We might end up needing to use a path option, e.g. --path [path].

Thoughts about this and next steps?

cjreimer commented 3 years ago

David, I think we're on the same page!

I'd gladly take this on except it would probably sit in my queue for a while as I have a lot on my plate for August. Feel free to post a 'help wanted' flag once you think there's been enough review on the subject. If no one else picks it up, I'll add it to my list.

thedavidprice commented 3 years ago

Update, possible that @jtoar will add this to the current #2606

redwoodjs / redwood