Conversation
evantahler
left a comment
evantahler
left a comment
There was a problem hiding this comment.
This is a. great start! Well done! I've scattered some line comments below, but overall:
- Based on a common tutorial pattern, which is probably a good idea!
- Clear and easy to read
- I think naming the model
Taskmight be confusing, as Actionhero already has a main object called "Task". Perhaps renaming this toTODOorChoremight clear this up.
I think it's going to be important to add a UI as well, so new people can see what the end result it and not need to imagine it. Perhaps grabbing the source from https://github.com/tastejs/todomvc/tree/gh-pages/examples/react and checking it into /public to wire up to the API would be a good idea?
At the end of this, we can publish this to todo.actionhero.js and have a working demo!
|
|
||
| 4. Configure the database connection in `./src/config/sequelize.js` and run migrations. | ||
| ``` | ||
| npx sequelize-cli db:migrate |
There was a problem hiding this comment.
automigrate is enabled in config/sequelize.ts, so this isn't needed
There was a problem hiding this comment.
But doesn't it require a restart of dev server?
That is why I added it, Will remove the whole sequelize-cli thing, it was conflicting anyways.
There was a problem hiding this comment.
I've left it there but added the comment about auto-migration feature available in AH.
Let me know if it works for you otherwise I'll remove it.
README.md
Outdated
|
|
||
| 5. Start Dev server and access API's at <a href="http://localhost:8080">Localhost</a> | ||
| ``` | ||
| npm run test actions/task |
There was a problem hiding this comment.
What does this test command do? Looks like just normal npm test?
There was a problem hiding this comment.
Yes, this just specifically tests actions/task.ts.
Since we are following the tutorial, we first test models/task.ts and then actions/task.ts
|
|
||
| ``` | ||
| npx actionhero generate | ||
| ``` |
There was a problem hiding this comment.
Might want to note what this command does - creates a new Actionhero project, package.json, etc.
README.md
Outdated
|
|
||
| Start the server by running: | ||
|
|
||
| `npm start` |
There was a problem hiding this comment.
It's probably better to get folks into the habit of running npm run dev instead - this way they don't need to worry about compiling their changes
README.md
Outdated
|
|
||
| Check the directory structure and note the following: | ||
|
|
||
| - The preconfigured scripts are available in `todo/package.json` |
There was a problem hiding this comment.
just pacakge.json i think (relative to project root)
| ``` | ||
|
|
||
| This in turn makes the CLI use configuration from `src/config/sequelize.js`. | ||
|
|
There was a problem hiding this comment.
At some point, they will need to make a database createdb actionhero or similar
README.md
Outdated
|
|
||
| ``` | ||
|
|
||
| This code is self explanatory but for further clarity the above test checks if a new task gets created in the database with the required properties. |
There was a problem hiding this comment.
Please try to avoid language like "self explanatory" - this could be insulting to a new reader who doesn't understand it (this is likely a cultural thing). I tend to say something like:
This code checks if a new task gets created in the database with the required properties. To learn more about Jest tests, visit the jest website
src/config/sequelize.ts
Outdated
| let database = "ahtodotutorial"; | ||
| let username = "ymmy"; | ||
| let password = ""; |
There was a problem hiding this comment.
I think hard-coding a database name is OK if you tell people to create a database of the same name in the README. I'd remove the name here. Postgres, by default, will use the currently logged in user, which is normally a safe default.
There was a problem hiding this comment.
Removed this for now. I've added in the description about replacing it with their own configurations.
|
It also seems you are missing the migration to create the Task model (pending rename) |
- Tasks are internally used in AH, hence for clarity purposes we are renaming Todo to Bucket List and Task Models and Actions to Goals
|
I've made the necessary changes to the backend. Will work next on the demo frontend react-app. |
|
Hello @y471n! I wanted to check in and see how this was going. Anything you need from me? |
|
@y471n is it safe to assume this project is abandoned? |
|
I'm going to archive this project. We can un-archive it if you come back! |
2 participants
No description provided.