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

Enable custom response support for callback functions #183

Closed
wants to merge 1 commit into from

Conversation

oryba
Copy link

@oryba oryba commented Dec 30, 2017

Based on issue #182 (Set a cookie from callback function).

With the following functionality callback function can return not only Dash components tree, but a customizable DashResponse with cookies, headers etc.

from dash import DashResponse
...

@app.callback(
    dash.dependencies.Output('content-block', 'children'),
    [dash.dependencies.Input('comparison-filter', 'value')])
def on_filter_change(comparison_value):
    response = DashResponse(html.Div('foo', className="test"))
    response.set_cookie('bar', 'baz')
    return response

@chriddyp
Copy link
Member

chriddyp commented Jan 2, 2018

Yeah, I like this solution. An alternative way to solve #182 would be to implement a Cookie Dash component. However, this solution is even more flexible by allowing things like headers.

class DashResponse(Response):
"""
Flask Response extended with option to convert a regular response to
valid Dash json-encoded
Copy link
Member

Choose a reason for hiding this comment

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

missing "component" here? How about:

Flask Response extended with option to convert a regular response to
valid Dash json-encoded component.
Return a DashResponse object from a Dash callback in order to set
other properties of the response like headers or cookies

}
self.set_data(
json.dumps(response, cls=plotly.utils.PlotlyJSONEncoder)
)
Copy link
Member

Choose a reason for hiding this comment

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

could we make this code a little bit more DRY by combining it in some way with these lines?

dash/dash/dash.py

Lines 494 to 506 in b61f0d3

response = {
'response': {
'props': {
output.component_property: output_value
}
}
}
return flask.Response(
json.dumps(response,
cls=plotly.utils.PlotlyJSONEncoder),
mimetype='application/json'
)

@chriddyp
Copy link
Member

chriddyp commented Jan 2, 2018

Looks good! In order for this to be merged, I'd like to see:

  • The DRY component addressed
  • A test

I think that a full integration test would be best suited here. Most of the integration tests are in the dash-renderer library (https://github.com/plotly/dash-renderer/blob/cef2313478ee2c37b91b9ef773bf6991bc96240e/tests/test_render.py). We should add an integration test to this library as well. I can make a separate PR to add a simple integration test and then we can rebase this PR off of that branch and add another integration test

@chriddyp chriddyp mentioned this pull request Jan 3, 2018
@chriddyp
Copy link
Member

chriddyp commented Jan 3, 2018

I have merged #184 , so let's rebase this PR off of that and then add an integration test.

@oryba - Do you have the time to wrap up this PR from my comments above or would you like one of the core contributors to do so?

@oryba
Copy link
Author

oryba commented Feb 13, 2018

@chriddyp - unfortunately, now I have no time to do this, but I'm going to go back to this PR as soon as my current project will be finished

@jkseppan
Copy link

Hey @oryba - I'm interested in this feature and I'm willing to work on it, so if you don't mind I could try to add the requested test and do the refactoring?

@oryba
Copy link
Author

oryba commented Sep 24, 2018

Hey @jkseppan, sorry for the late answer. I don't mind, thanks for your help, glad to see someone has a chance to finalize this feature.

@alexcjohnson
Copy link
Contributor

Superseded by #401 - thanks @oryba and @jkseppan! We still need to get the new one merged but I believe this older PR can be closed.

HammadTheOne pushed a commit to HammadTheOne/dash that referenced this pull request May 28, 2021
HammadTheOne pushed a commit to HammadTheOne/dash that referenced this pull request May 28, 2021
* - update sort_settings to use column_id

* update tests with column_id

* update doc
HammadTheOne pushed a commit that referenced this pull request Jul 23, 2021
* - update sort_settings to use column_id

* update tests with column_id

* update doc
HammadTheOne pushed a commit that referenced this pull request Jul 23, 2021
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.

None yet

4 participants