Skip to content

Add support for ES module (import/export syntax) callbacks #12812

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

Merged
merged 10 commits into from
May 31, 2023

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Feb 17, 2023

Add support for ES modules (static import and export syntax) to custom JS callbacks. The main reason for this is to allow external module imports. The example is adapted from #12808. CustomJS.code can be two things now:

  1. Function body as before. The only differences are that the function is cached and compiled only when code or args change, and can return a Promise.
  2. An ES module indicated by using export default syntax, e.g.:
export default function(args, obj, data) {
  // do something with the inputs
}

The exported default function can be a function, async function, arrow function () => ..., async arrow function async () => ..., or even a generator function. Arguments to the function are fixed and independent of CustomJS.args. One can take advantage of object decomposition to reveal the passed-in arguments, e.g.:

export default function({data_source, slider}, obj, data) {
  data_source.data.x.push(slider.value)
}

Additionally there's CustomJS.from_file(path, **args) to allow creating JS callbacks from source files (supported file extensions are *.js and *.mjs).

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #12812 (d5b589c) into branch-3.2 (e1d781e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           branch-3.2   #12812   +/-   ##
===========================================
  Coverage       92.43%   92.43%           
===========================================
  Files             315      315           
  Lines           20054    20070   +16     
===========================================
+ Hits            18537    18552   +15     
- Misses           1517     1518    +1     

@bryevdv
Copy link
Member

bryevdv commented Feb 17, 2023

I am pretty 👎 on a new thing and especially on that thing being called CustomESM since I suspect that "ESM" does not have much or any meaning for most Bokeh users, and forces users to make a choice that they aren't well equipped to evaluate. I would really like to have justification for not figuring out how to extend or improve existing CustomJS since I think that would be much friendlier for users.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 18, 2023

Given that there's this many CustomJS variants:

src/bokeh/models/callbacks.py:class CustomJS(Callback):
src/bokeh/models/expressions.py:class CustomJSExpr(Expression):
src/bokeh/models/filters.py:class CustomJSFilter(Filter):
src/bokeh/models/formatters.py:class CustomJSTickFormatter(TickFormatter):
src/bokeh/models/tools.py:class CustomJSHover(Model):
src/bokeh/models/transforms.py:class CustomJSTransform(Transform):

then I will have to figure out how to not duplicate all of the above. In fact I would like the module variant to become the default and phase out the existing variant. I will probably just detect export default, as exports can only be located at the top module level, which should be simple enough to avoid including a full JS parser.

@gmerritt123
Copy link

Another idea/option would be to provide the ability to declare imports not within CustomJS snippets but to apply them to the entire curdoc, i.e. a lot like what I'm doing by embedding CDN URLs in the template. I don't wanna have to import d3 in every snippet of CustomJS I write for an application, I wanna have access to it in every single one.

Maybe having both options would be ideal so you could limit the scope of certain imports if you want?

@mattpap mattpap force-pushed the mattpap/custom_esm branch from a6e1bc3 to 1b0e0de Compare March 1, 2023 23:32
@mattpap mattpap changed the base branch from branch-3.1 to branch-3.2 March 4, 2023 15:47
@mattpap mattpap mentioned this pull request Mar 10, 2023
4 tasks
@mattpap mattpap force-pushed the mattpap/custom_esm branch from 1b0e0de to 740080f Compare March 28, 2023 10:28
@mattpap mattpap changed the title Add support for CustomESM callbacks Add support for ES module (import/export syntax) callbacks Mar 28, 2023
@mattpap
Copy link
Contributor Author

mattpap commented Mar 28, 2023

Another idea/option would be to provide the ability to declare imports not within CustomJS snippets but to apply them to the entire curdoc, i.e. a lot like what I'm doing by embedding CDN URLs in the template. I don't wanna have to import d3 in every snippet of CustomJS I write for an application, I wanna have access to it in every single one.

We could set up an importmap (see MDN). However, browser vendors are dragging their feet implementing this, especially in Safari, so it's not a viable option yet. We can implement our own import mapping with URL objects and a bit of JS parsing.

@mattpap mattpap force-pushed the mattpap/custom_esm branch 3 times, most recently from b678e51 to 013930a Compare March 29, 2023 09:51
@mattpap mattpap force-pushed the mattpap/custom_esm branch from 013930a to 87d4640 Compare April 6, 2023 22:25
@mattpap mattpap force-pushed the mattpap/custom_esm branch from 87d4640 to ddabf65 Compare May 21, 2023 16:15
@mattpap
Copy link
Contributor Author

mattpap commented May 21, 2023

This is ready for review. For now I added support only to CustomJS, because that fits nicely into async nature of ESM handling. Other types of CustomJS are on code path not compatible with async, so they will require some more though how to handle things. This will probably require deeper "asyncification" of bokehjs, require anyway in other contexts (e.g. ipywidgets integration).

@mattpap
Copy link
Contributor Author

mattpap commented May 21, 2023

export default function(args, obj, data)

The order of arguments is up to debate. Probably a better order would be args, data, obj, given how rarely obj is used. Note that ESM variant doesn't bind this, so this will not point to obj. Note also that it's very intentional that args and data are not spread in function's signature. This guarantees a single signature in all contexts, contrary to what we have now with classic CustomJS. This also isn't inconvenient, because one case use JS object spread syntax to reveal members of args and data, e.g.:

export default function({source, slider}, _, {geometry: {sx, sy}}) {
  ...
}

Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

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

Couple of comments but LGTM

return true
default:
return false
}
Copy link
Member

Choose a reason for hiding this comment

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

incredible

@mattpap mattpap force-pushed the mattpap/custom_esm branch from 02faa58 to d5b589c Compare May 31, 2023 07:35
@mattpap mattpap merged commit 39146d3 into branch-3.2 May 31, 2023
@mattpap mattpap deleted the mattpap/custom_esm branch May 31, 2023 10:13
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
…#12812)

* Add support for CustomESM callbacks

* Allow callbacks to be asynchronous

* Merge ESM support into CustomJS

* Add support for CustomJS.from_file()

* Add release notes

* Make sure unzip([]) doesn't fail

* Add AsyncGeneratorFunction to core/types

* Allow isFunction() to detect generator functions

* Update CustomJS' unit tests

* Add unit tests for `CustomJS.from_file()`
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants