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 unminified components bundle support. #369

Merged
merged 13 commits into from
Sep 21, 2018
Merged

Add unminified components bundle support. #369

merged 13 commits into from
Sep 21, 2018

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Sep 6, 2018

Add support for serving unminified component bundles. Resolves #313.

Enable with:

app = dash.Dash(__name__, dev_tools_serve_dev_bundles=True)

The components libs needs a new key in the their _js_dist: dev_package_path.

  • dash-html-components
  • dash-core-components
  • dash-renderer

@rmarren1
Copy link
Contributor

rmarren1 commented Sep 7, 2018

This looks great! I think we should actually turn this feature on when a user is in a global "dev" mode, where they would also turn on the Dash Dev Tools features. I think an argument to dash.Dash is the most intuitive way to turn such a thing on.

I don't have a method in Dev Tools just yet. Maybe we should change serve_dash_bundles to something more general like use_dev_tools, then I can piggy back off this flag in the Dev Tools PR and turn those on when it is true.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 7, 2018

@rmarren1 So you want to combine the flag to serve the dev bundle and activate the devtools ? It make sense because the stacktraces will be much better with dev bundles.

I think we should call the keyword argument enable_dev_tools or just dev_tools.

@rmarren1
Copy link
Contributor

rmarren1 commented Sep 7, 2018

Yeah either of those sound good. Yeah the stack traces are generally unreadable for the production bundles, so Dev tools should definitely depend on them

@chriddyp
Copy link
Member

chriddyp commented Sep 7, 2018

I think an argument to dash.Dash is the most intuitive way to turn such a thing on.

One issue with this is that users will accidentally deploy with dev_tools turned on. I'd also like for this to become the default behaviour for dash. So, I think we should introduce a second set of config parameters in the run_server call that turn on all dev_tools-like things. Users shouldn't be doing run_server in production as they should be running it with gunicorn. We could even move towards renaming run_server to like run_dev_server or something.

For some more context, see the debug, dev, production section in this issue: #312

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 7, 2018

For the same reason @chriddyp mentionned, I don't really like the argument in the constructor for this (and hot-reload). I think these arguments could very well be in run_server since that the most likely place it gonna be used and have them default to true.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 7, 2018

@chriddyp I made the change to run_server, please review.

@rmarren1
Copy link
Contributor

rmarren1 commented Sep 7, 2018

💃 looks good to me!

@ned2
Copy link
Contributor

ned2 commented Sep 8, 2018

I like the approach of pushing dev mode activation away from Dash's constructor. I also think it's important that we don't bake dev-mode specific setup logic into run_server because users should still be able to conveniently run in dev mode using a WSGI server like gunicorn etc. This enables development/debugging in an environment that is closer to production for those people not using the built-in development server.

What about having a separate Dash method like activate_dev_mode or something that does all the relevant setup (I guess currently just setting _serve_dev_bundle to True, but I can imagine the setup might grow over time) and which run_server can call? Then this could be manually invoked for those wanting dev mode on gunicorn etc?

If we wanted to do that, this could be a separate PR though.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 17, 2018

@chriddyp @rmarren1 @ned2

I changed the naming of the dev tools variables, please review.

Also I used _config.get_config in lieu of with_defaults, this allow for setting the dev tools variables from environ, can use them to have different run configs without changing the code.

@T4rk1n T4rk1n force-pushed the serve-unminified branch 2 times, most recently from a1cf21e to b5fe3c8 Compare September 20, 2018 16:30
@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 20, 2018

I released the component suites bundles, when I tried to load them from unpkg it failed because of the name change from bundle.js to lib_name.min.js

Fix:

  • dash-core-components
  • dash-html-components
  • dash-renderer

dash/dash.py Outdated
served by wsgi and you want to activate the dev tools, you can call
this method out of `__main__`.

:param debug: If false no tools will be activated.
Copy link
Member

Choose a reason for hiding this comment

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

Should we add If True, then all of the other dev tool settings will be True unless specifically overridden.

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for your patience on this one!

@T4rk1n T4rk1n merged commit f94991b into master Sep 21, 2018
@T4rk1n T4rk1n deleted the serve-unminified branch February 1, 2019 02:41
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.

4 participants