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

Inline clientside callbacks #967

Merged
merged 12 commits into from
Nov 15, 2019
Merged

Conversation

jjaraalm
Copy link
Contributor

@jjaraalm jjaraalm commented Oct 16, 2019

Addresses at least part of #956 and adds support for writing JS clientside callbacks in python source.

Adds explicit JS source injection through a new keyword argument to Dash.clientside_callback:

clientside_callback(
    ClientsideFunction('namespace_to_inject_to', 'function_to_inject'),
    Output('output', 'value'),
    [Input('input', 'value')],
    source="""
    function(value) {
        console.log(value);
        return parseInt(value, 10) + 1;
    }
    """
)

Also adds source injection via the Dash.callback decorator. This probably should be explained or documented better so that users don't think they can write arbitrary python code here. Eventually I would like to add support for transpiling python -> JS, but this is just a stepping stone.

@callback(
    Output('output', 'value'),
    [Input('input', 'value')],
    clientside=True
)
def native_js_callback(value): 
    # Interior source is JS that will be added to the 
    # dash_clientide._inline_callbacks namespace.
    # Single-line comments are properly escaped.
    console.log(value);
    return parseInt(value, 10) + 1;

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Add inline clientside callbacks to Dash.clientside_callback
    • Add decorated clientside callbacks to Dash.callback
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow
    • this github #PR number updates the dash docs
    • here is the show and tell thread in plotly dash community

@alexcjohnson
Copy link
Collaborator

@jjaraalm you're on a roll 🏎

I very much like your implementation with inline_scripts, that's simpler than what I was envisioning 🎉 - that said if it's going to be in config - and thereby intentionally exposed to users - we would need to (a) include it in the Dash constructor args, and (b) document it fully. For now, unless there are clear use cases for users to add other things to it, let's keep it private and out of config, just set it as something like self._inline_scripts.

I'd like to see if we can get rid of the source kwarg, and just replace the ClientsideFunction first arg with the function source string, as proposed in #956. This would require choosing an implicit namespace and function name. Is there a reason to want explicit names? I suppose in principle that could allow multiple callbacks to use the same function, but that feels to me like an unnecessary optimization that will lead to more confusing code, especially since such sharing would only generally be useful for very short functions. I'd propose a namespace like _dashprivate, and the name could be just the (first) output id and prop, joined with maybe a double underscore, ie output__value?

I'm not a fan of writing JS code directly in a Python function body, and I don't see how it can be a stepping stone without later becoming a breaking change. If it's alright with you, let's leave this flavor out unless and until we're ready to do the transpiled version.

@jjaraalm
Copy link
Contributor Author

Thanks for the feedback. I wasn't thinking so much about the implications of putting it in config, I just liked the symmetry with external_scripts. I could see why some people might use it in the constructor (to avoid writing simple JS assets), but I agree it probably shouldn't be there, I'll fix that.

I agree with your assessment of the utility of namespace/function name for inline scripts. Really I only see that being used for intricate, overly complex, and probably not well-documented callback ... which I'm guilty of sometimes haha.

I think JS in python feels dirty, but that said I dislike code-in-strings even more. It's more clear that it's a foreign language, but also there's no easy way to get syntax highlighting or linting in editors (that I know of). I've always found code-in-strings to be a pain to write and debug. I understand your points though, and we can leave it out.

@jjaraalm
Copy link
Contributor Author

I decided to create a new namespace prefixed by _dashprivate_ for each output id. This is to prevent the highly unlikely case where someone creates a custom component with horrible prop names then decides to use even worse id names which result in a conflict.

`dash.dependencies.ClientsideFunction(namespace, function_name)`
argument that describes which JavaScript function to call
(Dash will look for the JavaScript function at
`window[namespace][function_name]`).
`window.dash_clientside[namespace][function_name]`), or it may take
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch! 🔬

dash/dash.py Outdated Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

Alright, Looks good to me! Just needs a changelog (and updating so it doesn't conflict with your other PR 😏) and we'll be ready to go.

@Marc-Andre-Rivet
Copy link
Contributor

@alexcjohnson @jjaraalm Looking into the DashR parity implementation and came to the conclusion that we wanted to prepend the namespace with _dashprivate_ -- glad to see you've already thought of everything! This looks great. As previously stated, just needs a changelog and an update 🎉

@Marc-Andre-Rivet Marc-Andre-Rivet modified the milestones: Dash v1.6, Dash v1.7 Nov 1, 2019
@jjaraalm
Copy link
Contributor Author

jjaraalm commented Nov 7, 2019

Sorry guys, was traveling. This should take care of it I think.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Love it, thanks for the finishing touches. 💃
I'm going to hold off merging for a little bit though in case we want to make a patch release on Monday's 1.6.0

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

💃 @jjaraalm Took the liberty of merging in dev changes. Will merge it in once the build passes one last time. Again thanks for the contribution! The next planned release is 1.7.0 around 11/25.

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit 5d9f578 into plotly:dev Nov 15, 2019
@ieipi
Copy link

ieipi commented Dec 18, 2019

I've updated to dash=1.7 and want to use the new inline callbacks feature, but got the following error:

TypeError: clientside_callback() got an unexpected keyword argument 'source'
(base)

and when i want to use the native_js_callback syntax, it gave the error:

callback() got an unexpected keyword argument 'clientside'

@alexcjohnson
Copy link
Collaborator

@ieipi the final implementation is not as described in the lead comment for the PR. See https://community.plot.ly/t/dash-v1-7-0-released/31961 for an example.

HammadTheOne pushed a commit to HammadTheOne/dash that referenced this pull request May 28, 2021
…ted-git-info-2.8.9

Bump hosted-git-info from 2.7.1 to 2.8.9
HammadTheOne pushed a commit that referenced this pull request Jul 23, 2021
…t-info-2.8.9

Bump hosted-git-info from 2.7.1 to 2.8.9
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