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

Allow to serve extensions' bundles and related resources #9799

Merged
merged 1 commit into from Mar 25, 2020

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Mar 20, 2020

So far, extensions compiled ahead of time (new-style extensions) can be embedded only inline. This PR allows to serve compiled bundles by bokeh server and consume extensions' resources from CDN (unpkg.com). This is an early preview and I will need to redesign this to allow serving other resources (e.g. fonts). This is needed to finalize bokeh/ipywidgets_bokeh#5.

@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Mar 24, 2020

CDN support will have to be tested and possibly improved upon when ipywidgets_bokeh's package is up on npm.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Mar 24, 2020

CDN support will have to be tested and possibly improved upon when ipywidgets_bokeh's package is up on npm.

Please describe what you mean here in some detail.

@@ -68,7 +68,7 @@ def append_version(cls, path):
return path
else:
Copy link
Member

@bryevdv bryevdv Mar 25, 2020

Choose a reason for hiding this comment

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

There are some out of date comments in this file, as long as you are tidying up they could be removed/improved?

#
# The full license is in the file LICENSE.txt, distributed with this software.
#-----------------------------------------------------------------------------
''' Provide a request handler that returns a page displaying a document.
Copy link
Member

@bryevdv bryevdv Mar 25, 2020

Choose a reason for hiding this comment

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

This docstring is copy-pasta

extensions = []
bundles = []

extensions = [".min.js", ".js"] if resources.minified else [".js"]
Copy link
Member

@bryevdv bryevdv Mar 25, 2020

Choose a reason for hiding this comment

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

Any reason not to make these immutable?

pkg_version = pkg.get("version", "latest")
pkg_main = pkg.get("module", pkg.get("main", None))
if pkg_main is not None:
cdn_url = f"https://unpkg.com/{pkg_name}@^{pkg_version}/{pkg_main}"
Copy link
Member

@bryevdv bryevdv Mar 25, 2020

Choose a reason for hiding this comment

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

can we make the CDN base URL configurable? I can imagine folks wanting to use different ones (e.g I've never heard of this one, but have heard cdnjs mentioned often)

Copy link
Contributor Author

@mattpap mattpap Mar 25, 2020

Choose a reason for hiding this comment

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

At some point. CDN handling is a secondary concern of this PR.

Copy link
Member

@bryevdv bryevdv Mar 25, 2020

Choose a reason for hiding this comment

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

At the very least pull it in to a module level attr so that there is not an arbitrary constant value buried in code.

@mattpap mattpap force-pushed the mattpap/extensions_handler branch from a18053b to 211b7b3 Compare Mar 25, 2020
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Mar 25, 2020

This pull request introduces 1 alert when merging 211b7b3 into 40e13e3 - view on LGTM.com

new alerts:

  • 1 for Unnecessary 'else' clause in loop

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Mar 25, 2020

The alert about for...else is valid and should be addressed. That construction is unusual and rare enough to warrant a comment even when it is useful to use, but without a break it is just a confusing no-op

@mattpap mattpap force-pushed the mattpap/extensions_handler branch from 211b7b3 to 4f1039e Compare Mar 25, 2020
@mattpap mattpap merged commit b44c236 into master Mar 25, 2020
19 checks passed
@mattpap mattpap deleted the mattpap/extensions_handler branch Mar 25, 2020
dist_dir = join(base_dir, "dist")

ext_path = join(base_dir, "bokeh.ext.json")
if not exists(ext_path):
Copy link
Contributor

@philippjfr philippjfr Apr 1, 2020

Choose a reason for hiding this comment

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

This caused a major regression in Panel, we did not bundle the bokeh.ext.json or package.json until now. Should have reviewed this more closely.

Copy link
Contributor Author

@mattpap mattpap Apr 2, 2020

Choose a reason for hiding this comment

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

package.json is still optional, though useful to have if one wants to publish an extension to npm. However, bokeh.ext.json has been a part of the, let's call it, API since the new compiler was introduced, but wasn't used by embed code so far, which was a bug in itself. We need to set up a better testing and review process (you can see what the review focused on).

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

3 participants