Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add api generator watch mode #3447

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open

Conversation

manuelblum
Copy link
Collaborator

depends on: #3446

Description

adds --watch option to comet api-generator --watch which watches for changes on entity.ts files. If changes occur - the necessary generated files for the entity get regenerated.

adds --file option to comet api-generator -f src/products/entities/product.entity.ts

Screenshots/screencasts

Screen.Recording.2025-02-14.at.13.17.03.mov

Further information

@auto-assign auto-assign bot requested a review from johnnyomair February 14, 2025 12:34
@manuelblum manuelblum force-pushed the feat/api-generator-watch-mode branch from 1f16988 to c5c1c0f Compare February 14, 2025 12:37
@manuelblum manuelblum changed the title Feat/api generator watch mode Feat: api generator watch mode Feb 14, 2025
@manuelblum manuelblum changed the title Feat: api generator watch mode Add api generator watch mode Feb 14, 2025
childProcesses[path].kill();
}
delete childProcesses[path];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it would be more safe to await the kill to be finished. Not that two processes (or even more if many change events come in for the same file) write the same files.

In practice - due to the slowness of the generator - it might be never a problem.

});

child.on("close", (code) => {
if (errorOutput.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the exit code as source for sucess/failure

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes true, changed this.

@manuelblum manuelblum force-pushed the feat/api-generator-watch-mode branch from 3887bbf to b8ce4d6 Compare February 14, 2025 13:51
@kaufmo kaufmo force-pushed the feat/api-generator-watch-mode branch from b8ce4d6 to 810368c Compare February 14, 2025 20:07
@kaufmo kaufmo force-pushed the feat/api-generator-watch-mode branch from 4cddd17 to 8e99627 Compare February 17, 2025 04:39
return false;
},
})
.on("change", async (path) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does chokidar handle consecutive writes to the same file? Would it be possible to wait until the child process has finished? The we wouldn't need to kill "old" child processes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what problem do you see with killing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably that it may be as unreliable as the "old" way we restarted the Vite dev server, resulting in tangling Node processes. And doing lot's of useless work 🤷🏼‍♂️

But if we can reliably kill child process than there's no real problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

killing processes works perfectly fine, when done correctly.

What can cause problems are multiple child processes (a tree), that can happen for example with npm run xxx that itself starts another child process. If you kill the npm run process, it's child process doesn't receive a kill signal. (This is different from Ctrl+C on the cli, which gets sent to all child processes). (I had those issues in dev-process-manager)

When you have a single child process (should be the case here, though it should be tested explicitly) this won't be an issue.

@manuelblum manuelblum force-pushed the feat/api-generator-watch-mode branch from 8e99627 to 68e77fd Compare February 28, 2025 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants