Scaffold Generator File Organization #2613

cjreimer · 2021-05-23T16:12:12Z

This is a PR that addresses #2543 regarding scaffold file generation, but I believe it also addresses #340.

Summary of status:

Code changes
Cypress e2e tests
Documentation Changes
Tutorial Changes

EDIT ==> please see post starting at Jun 9, 2021 for the latest. There were significant changes in the plan on this one.

What it does is (using a Post model as an example):

Renames the component EditPostCell.js to PostEditCell.js
Renames the component NewPost.js to PostNew.js
Renames the component EditPostPage.js to PostEditPage.js
Renames the component NewPostPage.js to PostNewPage.js
Reorganizes the components into a single folder by default (with an option to retain the existing behavior). See below for details.
Adjusts the imports to the full format to address the new format and get rid of typescript import warnings (DX). See below for details.

Not included in this PR at this time:

Restructure of the page folders (other than renaming as described above)
- There appears to be existing logic regarding folder names having to match page names in the router or other areas of the codebase. More thought would be required if we would want to change this, which I'm not sure if we would want to after briefly looking at it.
Optimization of layouts, as suggested by @cannikin is deferred, possibly implementing a future set context to pass in props.

Detail for 5. Reorganizes the components into a single folder by default (with an option to retain the existing behavior):

For example, for a model named Post, it would create the following structure:

in /components/

Post/
- PostEditCell.js
- PostNew.js
- Post.js
- PostCell.js
- PostForm.js
- Posts.js
- PostsCell.js

For tests and story files (not currently available), I envision them to be in subdirectories.

If you want to retain the current individual folder structure, then set the following in the redwood.toml file.

[generate]
  scaffoldIndividualComponentFolders= true

Detail for 6. Adjusts the imports to the full format to address new format and get rid of typescript import warnings (DX)

The imports in the scaffolded files are now fully expressed:

For example:
import PostCell from 'src/components/PostCell'
is now
import PostCell from 'src/components/Post/PostCell'

jtoar · 2021-05-24T22:41:36Z

@cjreimer Thanks for this, it's looking great! Just tried it out locally and things definitely feel a lot more organized from the get go. It even works with the nested syntax for major organizational wins!

A bit of a warning: you might have to deal with some merge conflicts. There's a few scaffolds/generator-related PRs out there right now. If it should happen, let me know if you need help with that, more than happy to do those for you!

One point of feedback, the name of this toml key:

scaffoldIndividualComponentFolders

Setting this to true means I get the Post/ directory right? I think the "Individual" part is confusing me a little. What do you think of something along these lines instead?

[generate]
  nestScaffoldComponents = true

cjreimer · 2021-05-25T00:16:29Z

@jtoar

Thanks for the feedback. I can help with the merge conflicts. It looks like #2609 might have some things to reconcile with. Do you want that one into the main branch first, or this one?

As for the operation of the toml key:
scaffoldIndividualComponentFolders

Currently, if you leave the key out or set it to false you get the new default behavior as described in the initial post for this pr. If you set it to true, you get something that looks like the previous behavior other than the renaming of the components. You would get:

in /components/

PostEditCell/
- PostEditCell.js
PostNew/
- PostNew.js
Post/
- Post.js
PostCell/
- PostCell.js
PostForm/
- PostForm.js
Posts/
- Posts.js
PostsCell/
- PostsCell.js

I know I kind of argued against this behavior in issue #2543; however, as writing the code, I remembered that the rest of redwood by default has one folder per component, so I thought we should at least have this as an option. We could rename this toml option to: scaffoldOneFolderPerComponent

I'm not opposed to modifying the option, or even adding another option. Could you clarify what you envision nestScaffoldComponents = true would do relative to what the default option does?

packages/cli/src/commands/generate/scaffold/scaffold.js

jtoar · 2021-05-25T01:29:23Z

Do you want that one into the main branch first, or this one?

If we can get this one in today then let's do it! But no rush. I really only have on point of feedback below about the toml key's name.

Could you clarify what you envision nestScaffoldComponents = true would do relative to what the default option does?

My bad! I just wanted to rename the option scaffoldIndividualComponentFolders, not introduce a new one or anything. nestScaffoldComponents was my suggestion; what do you think of that one? scaffoldOneFolderPerComponent makes it sound like each component will be in it's own folder.

Having this option is great by the way! Thanks for going above and beyond to include it.

cjreimer · 2021-05-25T01:41:22Z

Thanks for the active and great feedback @jtoar !

scaffoldOneFolderPerComponent makes it sound like each component will be in its own folder.

Yes! That's what it does!

jtoar · 2021-05-25T01:59:43Z

Anytime! And ah maybe I'm confused then--doesn't it put all the components (Post, Posts, PostCell, PostCells, etc.) into one folder (Post)? scaffoldOneFolderPerComponent sounds like each component (Post, Posts, etc.) gets its own folder, which is already the case. oneComponentFolderPerScaffold sounds clearer but more verbose.

cjreimer · 2021-05-25T02:32:57Z

No problem ... I think we're just thinking the inverse case of what the flag should do. I can change the function of the flag to the inverse case, as I believe you are proposing. So, then we would have:

toml flag either missing [ default behavior ] or set as follows:
[oneComponentFolderPerScaffold] = true

==> Puts in new behavior and has all components in one folder by model For example:
Post/

PostEditCell.js
PostNew.js
Post.js
PostCell.js
PostForm.js
Posts.js
PostsCell.js

If the toml flag is set as follows:
[oneComponentFolderPerScaffold] = false

==> Retains previous behavior and has one folder per component

cannikin · 2021-05-25T02:57:59Z

Hmm, when we talked about the components going into a Post directory I assumed they would still be in their own subdirectories: Posts, NewPost, etc. While we don’t currently include tests and stories with the scaffold, we definitely should keep that folder structure so that they have a home when we’re ready.

Unless @mojombo disagrees, but this structure of a directory for each component was kind of one of the pillars of Redwood when we started. You could maybe argue that scaffolds are a special case but I’d have him weigh in on that if we plan on adding an option to let users do something different.

jtoar · 2021-05-25T03:31:36Z

Hmm, when we talked about the components going into a Post directory I assumed they would still be in their own subdirectories: Posts, NewPost, etc

Ah I didn't catch that; that's probably why I was confused about the name. As Rob said, we definitely want to keep them in subdirectories regardless, sorry we didn't make that clear!

jtoar · 2021-05-25T03:36:06Z

And @cannikin this example is probably why we all got confused:

I also okayed the changes that were clearly-spelled out in #2543 (comment), so totally on me!

cannikin · 2021-05-25T03:51:28Z

Ahhh crap sorry, that was my bad! Should have been the directory in there before the filename. 😬

…

On May 24, 2021, at 8:38 PM, Dominic Saadi ***@***.***> wrote: And @cannikin this example is probably why we all got confused: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

jtoar · 2021-05-25T04:06:30Z

@cjreimer Don't make any changes to this just yet, I'll get feedback from Tom and Peter tomorrow. I'm pretty sure they'll want to stick to the component-directory structure, but I'll ask them to be sure. Even if we do, we still like the change introduced in this PR of putting all those components in a Post/ directory. Thanks for working on this, and I owe you one!

cjreimer · 2021-05-25T12:55:19Z

No problem, @jtoar and @cannikin! Always pleased to help and give back.

I totally understand the desire to stick with the one component per folder structure, which has been consistently applied in Redwood to date. I can absolutely tweak the code to that behavior!

While I will defer to the core team on this, my personal preference is for tightly coupled components to be grouped into a single folder for that model/domain, with tests and stories in subdirectories (actually more similar to how the redwood framework is structured). With the above as the default case, would there be any support for having this structure as a non-default option requiring a manually set flag? I would see this non-default option getting expanded to the cells and component generators as well, where we specify a path (which could be an existing path), and the tests and stories get created in subfolders.

Either way, I'll get the code cleaned up once we have some direction.

Tobbe · 2021-05-25T15:35:43Z

This might not be the place, but FWIW I also think it might be time to take another look at how we organize/structure a RW project. It's getting kind of hard navigating my project by looking at the file tree with just a components and pages directory.

I think I'd like to see Cells and Components that are only used by one page to be nested with that page. And I guess nothing is stopping me from doing that, but maybe RW can do something about better supporting other ways to organize the code, or even change the way it currently does it by default.

Maybe this is a discussion better had in the forums...
I posted there as well. Feel free to add any more input anyone might have https://community.redwoodjs.com/t/perfect-file-structure-for-redwood-web/1575/9

cjreimer · 2021-06-10T13:49:41Z

@Tobbe, the changes have been made. Let me know if anything else is needed.

jtoar

@cjreimer was going to tell you the same thing as @Tobbe; glad you guys got that one already. Huge thanks for this! Besides just organizing our scaffolds, there's a lot of changes you made to the organization of the code here that's for the better (that us maintainers especially appreciate). Super glad we can get this one in!

cjreimer · 2021-06-10T16:21:54Z

Thanks, Dominic!

Tobbe · 2021-06-10T21:52:26Z

Any tutorial docs, tutorial project code or anything else that needs to be updated now?

thedavidprice · 2021-06-10T22:55:13Z

@Tobbe I had the same docs questions. On my list as well if not addressed here.

@jtoar @cjreimer curious if we tested this compatibility with yarn rw destroy scaffold .... Without looking, I believe there are unit tests that would have needed to be updated in this PR. But will destroy be backwards compatible?

update: it doesn't look like it is backwards compatible — not necessarily an issue but should be discussed

update 2: ah, I think I'm getting it now. You'd need to use the toml config appropriate to your setup. Confirmed it's working correctly for both create and destroy. Very nice. And I can handle in Release Notes. I like this a lot, btw.

[generate]
  nestScaffoldByModel= false

thedavidprice · 2021-06-10T23:02:19Z

@jtoar could you help update the current description and help output for yarn rw g scaffold --help:

rw g scaffold <model>

Generate Pages, SDL, and Services files based on a given DB schema Model. Also
accepts <path/model>

Positionals:
  model  Model to scaffold. You can also use <path/model> to nest files by type
         at the given path directory (or directories). For example, 'rw g
         scaffold admin/post'                                         [required]

Options:
      --help              Show help                                    [boolean]
      --version           Show version number                          [boolean]
      --tests             Generate test files                          [boolean]
  -f, --force             Overwrite existing files    [boolean] [default: false]
      --typescript, --ts  Generate TypeScript files   [boolean] [default: false]

cjreimer · 2021-06-10T23:04:36Z

@Tobbe Yes, we should probably update the tutorial. Some of the code extracts presented will need updating.

@thedavidprice I did test the destroy scaffold command reasonably extensively in my test project, and I believe it's working. I admit the automated tests aren't as thorough as the generate command.

As for backward compatible on the destroy command, you would need to add the following to redwood.toml.

[generate]
  nestScaffoldByModel= false

Is it confusing that the option says [generate] but would also apply to the [destroy]?

Alternatively we could look at adding an option into the command line option .. wouldn't be hard with how it's set up.

thedavidprice · 2021-06-10T23:07:47Z

@cjreimer Nice! I just figured that out locally as well and confirmed in an update to my original comment above. Super slick and I can communicate appropriately in the Release Notes. Thanks again 🚀

cjreimer · 2021-06-11T00:25:27Z

Docs updated in:
https://github.com/redwoodjs/redwoodjs.com/pull/715

Tobbe · 2021-06-11T19:38:09Z

@cjreimer Do you want to do the tutorial as well? If so, it's here https://github.com/redwoodjs/learn.redwoodjs.com
And the code lives here: https://github.com/redwoodjs/redwood-tutorial

cjreimer · 2021-06-11T20:08:28Z

@Tobbe, I can gladly tackle it, but please know that I have quite a backlog at the moment in my "real" work, so it might take me a couple of weeks to complete. Feel free to assign it to someone else if they have time. Otherwise, I'll gladly put it on my plate.

Do you have a best practice for making sure the code snippets and results presented are exactly as a new user would see? Do you run through the tutorial yourself or make the spot changes as per what you expect? I took a look, and already some of the existing routes stuff in the tutorial looks questionable as to whether it's the latest.

thedavidprice · 2021-06-11T22:07:18Z

@cjreimer @Tobbe I'll tackle the Tutorial changes. Rob is planning an overhaul once we have a stable v1-rc, so I might just go for "good enough" changes this time around. Regardless, it's on my list to handle.

Thanks, all!

cjreimer · 2021-06-11T22:15:59Z

Thanks @thedavidprice! I appreciate it!

callingmedic911 · 2021-06-16T08:38:18Z

Hi @cjreimer! I see couple of log when generating and deleting scaffold. I was wondering whether this is intentional or left from development? I will mark it the review page. Should we remove these or maybe rephrase if we have to keep it?

callingmedic911 · 2021-06-16T08:39:00Z

packages/cli/src/commands/destroy/scaffold/scaffold.js

@@ -64,6 +64,9 @@ const removeLayoutImport = ({ model: name, path: scaffoldPath = '' }) => {
    new RegExp(`\\s*${importLayout}`),
    ''
  )
+  console.log('regex:', new RegExp(`\\s*${importLayout}`))


Please see #2613 (comment)

callingmedic911 · 2021-06-16T08:39:12Z

packages/cli/src/commands/destroy/scaffold/scaffold.js

@@ -64,6 +64,9 @@ const removeLayoutImport = ({ model: name, path: scaffoldPath = '' }) => {
    new RegExp(`\\s*${importLayout}`),
    ''
  )
+  console.log('regex:', new RegExp(`\\s*${importLayout}`))
+  console.log(newRoutesContent)


Please see #2613 (comment)

callingmedic911 · 2021-06-16T08:41:45Z

packages/cli/src/commands/generate/scaffold/scaffold.js

+}) => {
+  if (typeof nestScaffoldByModel === 'undefined') {
+    nestScaffoldByModel = getConfig().generate.nestScaffoldByModel
+    console.log('nestScaffoldByModel from config: ', nestScaffoldByModel)


Please see #2613 (comment)

cjreimer · 2021-06-16T13:06:47Z

Hey thanks @callingmedic911. Yes, those should be removed. Thanks for catching that!
I'll get that cleaned up!

cjreimer added 4 commits May 22, 2021 23:00

revised scaffold file organization

68d3bd5

fixed scaffold tests

876f936

Merge branch 'main' into cjr-generator-scaffold-org

db946d0

scaffold changes

d263db3

cjreimer changed the title ~~Cjr generator scaffold org~~ Scaffold Generator File Organization May 23, 2021

fixed layout import

db4a69c

jtoar added this to the next-release-candidate milestone May 23, 2021

jtoar added topic/fully-integrated-dx topic: scaffold labels May 23, 2021

e2e tutorial updates

bcdea30

jtoar mentioned this pull request May 24, 2021

Doubly-nested scaffolds don't work with react-fast-refresh #2625

Closed

jtoar linked an issue May 24, 2021 that may be closed by this pull request

Suggestion - Redwood scaffold generator - file naming #2543

Closed

Merge branch 'main' into cjr-generator-scaffold-org

d181fdb

Merge branch 'main' into cjr-generator-scaffold-org

c51656d

jtoar reviewed May 25, 2021

View reviewed changes

packages/cli/src/commands/generate/scaffold/scaffold.js Show resolved Hide resolved

cjreimer added 2 commits June 10, 2021 08:01

Merge branch 'main' into cjr-generator-scaffold-org

c89f6c3

updated generated imports

7daaa48

cjreimer and others added 2 commits June 10, 2021 09:46

Merge branch 'main' into cjr-generator-scaffold-org

797e0c0

Merge branch 'main' into cjr-generator-scaffold-org

b40af29

jtoar approved these changes Jun 10, 2021

View reviewed changes

jtoar merged commit e561fe4 into redwoodjs:main Jun 10, 2021

thedavidprice added release:breaking This PR is a breaking change next-release/update-docs labels Jun 10, 2021

cjreimer mentioned this pull request Jun 11, 2021

Updated docs as per code changes in #2613 redwoodjs/redwoodjs-com-archive#715

Merged

thedavidprice modified the milestones: next-release-priority, next-release Jun 14, 2021

callingmedic911 reviewed Jun 16, 2021

View reviewed changes

cjreimer mentioned this pull request Jun 16, 2021

Removed development console.log lines #2844

Merged

pi0neerpat mentioned this pull request Jul 19, 2021

Add Auto-import for Nested Pages in App.js #3073

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scaffold Generator File Organization #2613

Scaffold Generator File Organization #2613

cjreimer commented May 23, 2021 •

edited

Loading

jtoar commented May 24, 2021

cjreimer commented May 25, 2021

jtoar commented May 25, 2021

cjreimer commented May 25, 2021

jtoar commented May 25, 2021

cjreimer commented May 25, 2021

cannikin commented May 25, 2021

jtoar commented May 25, 2021

jtoar commented May 25, 2021 •

edited

Loading

cannikin commented May 25, 2021 via email

jtoar commented May 25, 2021

cjreimer commented May 25, 2021

Tobbe commented May 25, 2021

cjreimer commented Jun 10, 2021

jtoar left a comment

cjreimer commented Jun 10, 2021

Tobbe commented Jun 10, 2021

thedavidprice commented Jun 10, 2021 •

edited

Loading

thedavidprice commented Jun 10, 2021

cjreimer commented Jun 10, 2021

thedavidprice commented Jun 10, 2021

cjreimer commented Jun 11, 2021

Tobbe commented Jun 11, 2021

cjreimer commented Jun 11, 2021

thedavidprice commented Jun 11, 2021

cjreimer commented Jun 11, 2021

callingmedic911 commented Jun 16, 2021

callingmedic911 Jun 16, 2021

callingmedic911 Jun 16, 2021

callingmedic911 Jun 16, 2021

cjreimer commented Jun 16, 2021

Scaffold Generator File Organization #2613

Scaffold Generator File Organization #2613

Conversation

cjreimer commented May 23, 2021 • edited Loading

jtoar commented May 24, 2021

cjreimer commented May 25, 2021

jtoar commented May 25, 2021

cjreimer commented May 25, 2021

jtoar commented May 25, 2021

cjreimer commented May 25, 2021

cannikin commented May 25, 2021

jtoar commented May 25, 2021

jtoar commented May 25, 2021 • edited Loading

cannikin commented May 25, 2021 via email

jtoar commented May 25, 2021

cjreimer commented May 25, 2021

Tobbe commented May 25, 2021

cjreimer commented Jun 10, 2021

jtoar left a comment

Choose a reason for hiding this comment

cjreimer commented Jun 10, 2021

Tobbe commented Jun 10, 2021

thedavidprice commented Jun 10, 2021 • edited Loading

thedavidprice commented Jun 10, 2021

cjreimer commented Jun 10, 2021

thedavidprice commented Jun 10, 2021

cjreimer commented Jun 11, 2021

Tobbe commented Jun 11, 2021

cjreimer commented Jun 11, 2021

thedavidprice commented Jun 11, 2021

cjreimer commented Jun 11, 2021

callingmedic911 commented Jun 16, 2021

callingmedic911 Jun 16, 2021

Choose a reason for hiding this comment

callingmedic911 Jun 16, 2021

Choose a reason for hiding this comment

callingmedic911 Jun 16, 2021

Choose a reason for hiding this comment

cjreimer commented Jun 16, 2021

cjreimer commented May 23, 2021 •

edited

Loading

jtoar commented May 25, 2021 •

edited

Loading

thedavidprice commented Jun 10, 2021 •

edited

Loading