Add path specification to scaffolds#423
Add path specification to scaffolds#423cannikin merged 4 commits intoredwoodjs:masterfrom jtoar:ds-dir-prefix
Conversation
|
That looks quite nice to me! As for the command-line itself, I'm wondering whether going for My reasoning is the following (hopefully I make it clear enough):
I think this kinda shows in the fact that everything generated on the So with that logic in mind, I'm thinking I'd still use a namespacing via nested paths as you did though, rather than prefixing the classes names. Otherwise I feel the |
|
@olance quick clarification:
I think
That's exactly what I'm interested too. I'll quote @cannikin here (pulling from the discourse thread)--I was basically implementing his suggestion since I thought it more declarative:
A flag was what I originally had in mind, but it was a |
Oooh my bad... Rails scaffold generator does create the model if it's missing, so I assumed Redwood's one would do the same! @cannikin what's your take on the |
|
FYI: we’ve added a CI runner for Windows and changed PR settings to require checks to pass before merging. Pulling in changes from master should resolve current warnings. |
|
@thedavidprice Thank you, resolved! |
|
@cannikin this one’s ready for review |
There was a problem hiding this comment.
I'd like to see some tests that exercise all this new code! You'll see we have tests in place that check both that file is created in the correct file path, and that the contents of the file match a fixture in the test directory.
I'd test the simple case of admin/post but also a multiword path like admin_pages/post and multiple directories like admin/pages/post.
Also does this cover all the possible cases, like admin/post and Admin/Post, or for multiword ones AdminPages/Post, adminPages/post, admin-pages/post...
|
@jtoar just checking back on this to make sure it doesn’t get lost in the shuffle. No urgency implied. But let us know if you’d like to take on the tests and need any help doing so. |
|
@thedavidprice thanks, I've started taking on the tests but could use a little feedback @cannikin. Here's a quick update:
I added some tests (
These tests are me literally copying your scaffold.test.js and adapting it to the new input. Which means I made more I couldn't get doubly-nested paths like The error I was getting:
I applied a fix in the |
|
@jtoar Yeah that's getting to be a lot of fixtures. I'd be happy with just testing that the file was created at the correct location, with a test like: expect(files).toHaveProperty([
'/path/to/project/web/src/pages/Admin/UsersPage/UsersPage.js',
])And then a test that the file content contains the expected import statement. Something like: expect(files['/path/to/project/web/src/pages/Admin/UsersPage/UsersPage.js'])
.toMatch(`import UsersCell from 'src/components/Admin/UsersCell'`)How does that feel? You can get rid of all those copied fixture files, sorry about that! |
|
@peterp Any ideas about this?
|
|
@cannikin Tests are here!
The structure of the tests are largely the same, so at first I tried to save my fingers by using jest's I need to do more research, but based on the way our
And I think that's what we do. So even though the tests seem to work, I'd be interested in knowing if there's a way I could've done them better! @peterp The fix is in - importStatement: `const ${importName} = { name: '${importName}', loader: () => import('${importFile}') }`,
+ importStatement: `const ${importName.split(',').join('')} = { name: '${importName.split(',').join('')}', loader: () => import('${importFile}') }`, |
|
@peterp Exactly. Routes.js has a preamble saying you can nest components in however many directories you want as long as you prefix the component with the directory names: // In this file, all Page components from 'src/pages` are auto-imported. Nested
// directories are supported, and should be uppercase. Each subdirectory will be
// prepended onto the component name.
//
// Examples:
//
// 'src/pages/HomePage/HomePage.js' -> HomePage
// 'src/pages/Admin/BooksPage/BooksPage.js' -> AdminBooksPageI found this to be not the case when it was nested in > 1 directory. I.e. if it was // 'src/pages/Super/Admin/BooksPage/BooksPage.js' -> SuperAdminBooksPageIt wouldn't work, the error being |
|
@jtoar is this ready for review? If so, please loop in Rob for the final check. Thanks! |
|
@cannikin This one's ready for another review. The fixture-less tests have been added and @peterp okayed the change made in internals. One last thing I wanted to make sure of: everyone prefers this syntax? The path/model syntax:
Instead of the path flag, which would be
|
|
+1 for path/model as much more intuitive re: DX. I have to think about --path=admin Haven't looked at the code. But would it be too much to accept either? |
|
Great! And I don't think that'd be too hard; I'll work on that in the mean time. |
|
@thedavidprice Added (tests still pass). I wrote it so that if the path's specified by both |
|
@jtoar I think that's great! Thanks for taking the time to figure that part out. For a next step after this PR, we are going to need documentation for this.
@cannikin this one looks good to me! However, I haven't run any local tests myself. And/or checked the new Jest tests. |
peterp
force-pushed
the
master
branch
4 times, most recently
from
f6a1008 to
a728f83
Compare
peterp
force-pushed
the
master
branch
2 times, most recently
from
711c520 to
2341368
Compare
|
@thedavidprice I can't get enough docs so I'll start the conversation up again over there. @cannikin I haven't changed the routes' names, so even if you generate the scaffold with something like Should the name now be |
I would expect so, yes. The |
This commit modifies scaffold.js to let the user specify a path via: - the argument (I.e. yarn rw g scaffold admin/user) - a path flag (E.g. yarn rw g scaffold user --path=admin) This entailed: 1) modifying the files/routes commands to accept a new (optional) param: scaffoldPath 2) defining a lot of constants like "pascalScaffoldPath" or "camelScaffoldPath" to plug in to the templates/routes.
In the Routes.js file, routes are imported automatically, and if they're nested, the user is just supposed to append the directory names to the component. That only works if it's nested "one-dir" deep. This fix lets it work for indefinite nesitng.
For the imports/routes I had to add/change the vars in the templates respectively.
Getting imports to work required me to add one--pascalScaffoldPath.
And for routes, I had to make that var in the scaffold.js file,
So I had to add new route vars--e.g. routes.${newRouteName}(), etc.
These three tests check to see if files are 1) named correctly, 2) imports correctly, and 3) "routed" correctly. They mostly reuse tests from the original scaffold.test.js. The tests cover three scenarios: 1) One-level nesting (scaffoldPath): 'admin/post' 2) One-level nesting, multiworld (ScaffoldPathMultiword): 'adminPages/post' 3) Two-level nesting (scaffoldPathNested): 'admin/pages/post'
|
@cannikin The route names are configured correctly now. I also tried to organize a bit--squashed/split everything into four commits. Sorry if I'm tripping over git while doing this (if I'm inconveniencing you guys please let me know!) Summary Just to summarize the work to date, you can now do something like: and/or and get this output: Basically the Expand to see sample tree output of web/src/I'm not sure if it'll add much value, but here's an example repo that has the output of running this modified scaffold command. Quick summary of the changes I made to
Tests I've added three tests that check to see if files are 1) named correctly, 2) imported correctly, and 3) "routed" correctly. They mostly reuse tests from the existing
You can run the tests by navigating to the I think we're getting close to the end? I'll keep looking the code over for mistakes/better ways to do things, but let me know if I can do anything else and thanks for your feedback! |
| export const handler = async ({ model, force }) => { | ||
| // The user can also specify a path in the argument. | ||
| // E.g. yarn rw g scaffold admin/post | ||
| export const handler = async ({ pathSlashModel, force, path: pathFlag }) => { |
There was a problem hiding this comment.
possible to add a Yargs 'description' key in here? It won't display correctly yet, e.g. yarn rw g scaffold --help currently shows help for yarn rw g only. But it helps code now and will be available via CLI in future once we re-structure Yargs:
https://yargs.js.org/docs/#api-optionskey-opt
There was a problem hiding this comment.
For the path flag, how does path to put generated files in sound? Alternatives:
path to generated filespath to nest generated files underdirectory(ies) to nest generated files under
And the command's description could change from/to Generates pages, SDL, and a services object => Generates and optionally nests pages, SDL, and a services object
|
Just ran through this locally -- it's really great behavior! I used the This really needs docs, especially because the @cannikin I did not review the code, just the behavior. Handing off to you! |
|
@thedavidprice reached out about docs at #310; will start a google doc. Stay tuned for an issue most likely later today. |
|
Great work! This has gotta be the biggest PR we've merged, 2,000+ lines 😮 |
* Add path specification via argument and flag
This commit modifies scaffold.js to let the user specify a path via:
- the argument (I.e. yarn rw g scaffold admin/user)
- a path flag (E.g. yarn rw g scaffold user --path=admin)
This entailed:
1) modifying the files/routes commands to accept a new
(optional) param: scaffoldPath
2) defining a lot of constants like "pascalScaffoldPath" or
"camelScaffoldPath" to plug in to the templates/routes.
* Add build-time route-import fix to @redwoodjs/internal
In the Routes.js file, routes are imported automatically, and if they're
nested,
the user is just supposed to append the directory names to the
component.
That only works if it's nested "one-dir" deep.
This fix lets it work for indefinite nesitng.
* Add/change vars in templates
For the imports/routes I had to add/change the vars in the templates respectively.
Getting imports to work required me to add one--pascalScaffoldPath.
And for routes, I had to make that var in the scaffold.js file,
So I had to add new route vars--e.g. routes.${newRouteName}(), etc.
* Add three path-specification tests
These three tests check to see if files are 1) named correctly,
2) imports correctly, and 3) "routed" correctly.
They mostly reuse tests from the original scaffold.test.js.
The tests cover three scenarios:
1) One-level nesting (scaffoldPath): 'admin/post'
2) One-level nesting, multiworld (ScaffoldPathMultiword): 'adminPages/post'
3) Two-level nesting (scaffoldPathNested): 'admin/pages/post'
|
Love this! Can't wait to put it to use. Thanks @jtoar ! |
* Add path specification via argument and flag
This commit modifies scaffold.js to let the user specify a path via:
- the argument (I.e. yarn rw g scaffold admin/user)
- a path flag (E.g. yarn rw g scaffold user --path=admin)
This entailed:
1) modifying the files/routes commands to accept a new
(optional) param: scaffoldPath
2) defining a lot of constants like "pascalScaffoldPath" or
"camelScaffoldPath" to plug in to the templates/routes.
* Add build-time route-import fix to @redwoodjs/internal
In the Routes.js file, routes are imported automatically, and if they're
nested,
the user is just supposed to append the directory names to the
component.
That only works if it's nested "one-dir" deep.
This fix lets it work for indefinite nesitng.
* Add/change vars in templates
For the imports/routes I had to add/change the vars in the templates respectively.
Getting imports to work required me to add one--pascalScaffoldPath.
And for routes, I had to make that var in the scaffold.js file,
So I had to add new route vars--e.g. routes.${newRouteName}(), etc.
* Add three path-specification tests
These three tests check to see if files are 1) named correctly,
2) imports correctly, and 3) "routed" correctly.
They mostly reuse tests from the original scaffold.test.js.
The tests cover three scenarios:
1) One-level nesting (scaffoldPath): 'admin/post'
2) One-level nesting, multiworld (ScaffoldPathMultiword): 'adminPages/post'
3) Two-level nesting (scaffoldPathNested): 'admin/pages/post'
6 participants
This PR proposes the following change to the
g scaffoldcommand:E.g. given the model user, running
yarn rw g scaffold admin/userwill output:Motivation
Scaffolds are amazing and could be a bit more amazing. The knock against scaffolds this PR addresses is their tendency to pollute the namespace.
The specific problem I had was suddenly having to contrive names for my app's real components and pages. I.e. I had a model called
notes, and scaffolding this out gave me a CRUD, but key names were taken:NoteandNotesas components,NotePageandNotesPagesas a pages, etc. I wanted to keep the scaffold as a CRUD while fleshing things out, but having to manually change everything seemed like a lot of work that I thought redwood should've made trivial from the get-go. Specifying a path can make the kind of admin organization seen in example-blog possible right away.There's an understandably tight link between a model's name and the scaffold generated. But this coupling must be loosened a bit. Specifying a path is one way of doing this. Another that's been thrown around involves a
--prefixflag to literally add a prefix to component and page names.I'll continue to scheme/push changes, but wanted to get this out to start a discussion/get help before moving too far in any one direction. Thanks/please help @cannikin @thedavidprice @olance.
Questions
Generating scaffold files...).