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

SocketIO.run() add additional reloader specific parameters. #1556

Closed
wants to merge 3 commits into from

Conversation

indietyp
Copy link

Currently, the SocketIO.run() method allows for some modifications of the reloader, specifically the ability to include extra_files, this pull request adds two new parameters to the SocketIO.run() method to further modify the behavior.

The exclude_patterns and reloader_type parameter, are just redirected to the reloader, but give SocketIO users the ability to further customize how the reloader functions.
This is helpful for projects that have huge e.g. asset folders in their project they would like to exclude, or where a specific reloader just does not work.

Currently, the SocketIO development server can only include additional files, but not selectively exclude them during debugging. 

The Werkzeug reloader already provides a parameter to be able to exclude specific patterns, leveraging that ability, SocketIO would be able to do the same. An additional parameter was added the run() method has the same behavior across extra_files, exclude_patterns, and reloader_type

The same has been done for the reloader_type additional argument. Werkzeug lets you choose which reloader to use and depending on the architecture/machine it may be beneficial to allow explicitly specifying the reloader used.
@miguelgrinberg
Copy link
Owner

miguelgrinberg commented May 22, 2021

I think the idea is good, but I'm not sure there is a reasonable way to implement this.

The socketio.run() method supports three different web servers. The solution that you created only works for gevent and eventlet, but is broken for the Flask dev server, which would ignore these arguments. This isn't your fault, the extra_files argument is already suffering from this problem, and I don't feel it is a good idea to make the problem even worse by including more options in this broken solution. Your solution also ignores a few more arguments that you can provide, including reloader_interval which is directly related to the two that you implemented in this PR.

I'm more inclined to say that if you want the full functionality of the reloader, then you should use the Flask dev server, and if you use a different server, then you get a minimal reloader. Supporting the reloader in a gevent/eventlet web server was just a measure that was necessary when the Flask dev server could not support WebSocket connections. This is now possible, so really during development there is no reason to use eventlet/gevent anymore.

@indietyp
Copy link
Author

Thank you very much for the feedback!
I have completely overlooked the reloader_interval and other arguments, my bad, sorry - in an ideal implementation all of the arguments would be supported, but I don't think that that is a realistic approach.

Currently all implementations (except the threading based one) construct a run_server() method, that is then called - this could easily be simulated for threading. What would you think about the externalization of the run_server() function creation into a separate method, that way people could still hook into the use_reloader with their own arguments and the current behaviour would not change.

What I propose:
Create a new method (something like _create_run_server()) which returns a callable, run() calls this method to retrieve the run_server() callable and then would call it, depending on the context with a reloader or not. People who would like to further modify the behavior could call _create_run_server() and then create the reloader themselves.

I know that that might not be the best solution, but thought it might the "best of both worlds". What do you think?

Thank you very much for your time and I would be willing to implement such a change - if desired.

@miguelgrinberg
Copy link
Owner

@indietyp Please have a look at the main branch and let me know if this works for you. I've added a reloader_options that replaces the extra_files argument. This is a dict in which you can add any arguments that are sent to the run_simple function in Werkzeug.

@indietyp
Copy link
Author

@miguelgrinberg yes that works for me. Thanks for adding the feature!

@indietyp indietyp deleted the patch-1 branch May 23, 2021 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants