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

fix: issue 784 router update start new server listening #787

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wainglaister
Copy link

Fix issue where router update creates a new server and replaces the old server but doesn't start the new server listening
Add config hot-reload to router when using a local config file (using the router update fix)

Motivation and Context

Fix corrects a mismatch between the documentation and implementation for router updating

Addition for hot-reload improves developer experience when running with a local config file

TODO

Add config hot-reload to router when using a local config file (using the router update fix)
@wainglaister
Copy link
Author

#784

@StarpTech
Copy link
Contributor

StarpTech commented May 28, 2024

Hi @wainglaister,

Thanks so much for your contribution! I've updated the comments for the methods. Unfortunately, we can't merge the PR just yet. It needs tests, and the watcher functionality should be properly abstracted into a package. Given this, the priority of this feature is quite low at the moment, and we don't have the resources to guide you through these changes or to ensure that this feature is working as expected.

To prevent similar situations in the future, we recommend creating an issue first and reaching an agreement there. Additionally, it would have been beneficial to separate the documentation update from the file watcher implementation. Thank you!

@StarpTech StarpTech added the internally-reviewed The issue has been reviewed internally. label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internally-reviewed The issue has been reviewed internally. router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants