feat: crud task according to workflow #19
Conversation
jean-michelet
force-pushed
the
task-workflow
branch
from
d2336d0
to
4bf50ea
Compare
| "start": "npm run build && fastify start -l info dist/app.js", | ||
| "build": "tsc", | ||
| "watch": "tsc -w", | ||
| "dev": "npm run build && concurrently -k -p \"[{name}]\" -n \"TypeScript,App\" -c \"yellow.bold,cyan.bold\" \"npm:watch\" \"npm:dev:start\"", | ||
| "dev:start": "fastify start --ignore-watch=.ts$ -w -l info -P dist/app.js", |
There was a problem hiding this comment.
Improve the dev mode experience. Inspired by: https://github.com/fastify/fastify-cli/blob/master/generate.js#L54
| @@ -10,6 +10,7 @@ export default async function serviceApp( | |||
| fastify: FastifyInstance, | |||
| opts: FastifyPluginOptions | |||
| ) { | |||
| delete opts.skipOverride // This option only serves testing purpose | |||
There was a problem hiding this comment.
As I feared, I think we've introduced a problem here: https://github.com/fastify/fastify-cli/pull/742/files#r1689541348
For example, @fastify/mysql passes all the given options directly to mysql2, which triggers warnings from mysql2:
Ignoring invalid configuration option passed to Connection: skipOverride. This is currently a warning, but in future versions of MySQL2, an error
will be thrown if you pass an invalid configuration option to a Connection
Maybe it's also a bad design to propagate all the options in the autoloader? Like the cli generated template suggest to do:
export default async function serviceApp(
fastify: FastifyInstance,
opts: FastifyPluginOptions
) {
await fastify.register(fastifyAutoload, {
dir: path.join(import.meta.dirname, "plugins"),
options: { ...opts } // opts propagated here
});
// code...
}
There was a problem hiding this comment.
Not sure what to say here, but you are right. Passing all default options that results into this error, is NOT good.
|
Sorry about my ignorance, but why do we need all those workflow code in a demo app? The CRUD code all seems fine to me. But what does all that workflow code do? Which has even an dependency outside this demo app with Why not just keep the demo clean, and focus on the CRUD part only? Or do I miss something. |
|
EDIT: component removed.
The workflow (state machine) is something I used professionally when I was a PHP developer and I think it's a relevant way to handle the life cycle of an entity. I needed it to my job where we handle entities workflows.
Yes, this is my package, this is more like a POC, I can create an official fastify package for it so it is an official dependency. But if you think it's inappropriate, too complex, I understand. Some tasks application have very free status transitions, some of them have strict rules. I tough it was an opportunity to add this feature and make discover a way of managing an entity's status. I am gonna wait for other reactions, thanks for your review 🙏 |
|
@gurgunday @climba03003 |
There was a problem hiding this comment.
IMO, the demo should follow some existing well defined schema.
For example https://petstore.swagger.io/
It shows authorization, linkage between entity, multipart handling, etc.
|
I can take inspiration from it, but maybe we can stick to a task application. I can complete this PR with users endpoints, I just wanted a feedback on a pure CRUD. Autorisation is for a next step. |
|
TODO:
|
|
Can do Users on an other PR. |
| @@ -41,11 +41,6 @@ To build the project: | |||
| npm run build | |||
| ``` | |||
|
|
|||
| In dev mode, you can use: | |||
| ```bash | |||
| npm run watch | |||
There was a problem hiding this comment.
It does work, but will people use it now that we run it concurrently with the dev server?
src/app.ts
Outdated
| customOptions: { | ||
| coerceTypes: "array", | ||
| removeAdditional: "all", | ||
| allErrors: true |
There was a problem hiding this comment.
I don't think we should enable allErrors.
Given it widen the attack factors.
Can you elaborate please ? |
Issue #15
I created a task CRUD with workflow configuration.