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

emit event when the Vite dev server has finished closing. #98

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

patreeceeo
Copy link
Contributor

@patreeceeo patreeceeo commented Dec 1, 2023

Currently

The Vite dev server instance is closed when I call the close method on the server instance returned by listen, however, the dev server uses resources (e.g. a port for the WebSocket) and those resources aren't freed synchronously.

Proposal

Emit another event when the dev server has finished closing so that the caller of listen knows when it's safe to use those resources (e.g. by calling listen again)

Context/Motivation

I want to have my backend be fully reloaded when its source code changes. Perhaps there are other ways to achieve this, but re-invoking listen and getting a new Server instance seems the most intuitive to me, right now.

Note: I spent a few minutes trying to get the build/CI for the project working but ended up testing these changes by copy/pasting main.ts into my project. These changes worked for me. Any guidance on how to do this properly would be appreciated! Also thanks for sharing this project!

@szymmis
Copy link
Owner

szymmis commented Dec 14, 2023

Hi @patreeceeo, I'm really sorry for such a late response.

I love how well written is the description of your use-case and a problem that you want to solve. I like the idea of using signals, because it's fully backwards compatible with previous versions.

The only thing that I would change is the event name to something like vite:close, so it's as short as possible and also consistent with the close event of the http server, what do you think?

Also, are you able to write a small note in README about this change? A few words in listen function section?
Let me know what you think, and thanks a lot for your contribution!

@patreeceeo
Copy link
Contributor Author

Sure sounds good!

README.md Outdated Show resolved Hide resolved
@szymmis
Copy link
Owner

szymmis commented Dec 19, 2023

I've added a comment with suggestion about rephrasing the README note but apart from that everything is perfect.
Let me know what you think about my suggestion!

EDIT: I've run the tests on this PR that failed due to the problems described in #99.
This issue was already fixed and you can just rebase to master.

patreeceeo and others added 5 commits December 19, 2023 15:04
This way, as a user of this library, you know when to safely restart the dev server without resource conflicts.
@szymmis
Copy link
Owner

szymmis commented Dec 20, 2023

Okay, everything is looking good, I'm merging it. I will release new version when #100 is merged. Thank you for your contribution @patreeceeo!

@szymmis szymmis merged commit abfd36a into szymmis:master Dec 20, 2023
2 checks passed
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.

2 participants