-
Notifications
You must be signed in to change notification settings - Fork 949
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 setting custom CDN for loading widgets. #2185
Conversation
Thanks! Do you know if any other CDN supports semver suffixes in URLs like unpkg? |
jsdelivr does. : ) |
Seems nice! Is there a natural place to add documentation for this? |
"declare var" fails with a "not defined" error when window.__widgets_cdn__ is not defined. but this seems to handle both cases well.
@SylvainCorlay Any concerns with merging this PR ..? |
@@ -1,6 +1,8 @@ | |||
// Copyright (c) Jupyter Development Team. | |||
// Distributed under the terms of the Modified BSD License. | |||
|
|||
const __widgets_cdn__ = (window as any).__widgets_cdn__ || 'https://unpkg.com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a bit cleaner if we make things more specific, like __jupyter_widgets_cdn__
, or if we could figure out a nice way to not have a global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the var name to __jupyter_widgets_cdn__
: )
I think it is ok like this, but indeed, maybe we can do without a global, ala requirejs <script type="text/javascript" data-cdn-prefix="https://unpkg.com" src="https://unpkg.com/@jupyter-widgets/html-manager@^0.14.0/dist/embed-amd.js"></script> I don't know how you can access this attribute, but requirejs seems to be able to do it. |
Is #2191 a good alternative? |
@maartenbreddels #2191 looks good to me, but I would let the team members @SylvainCorlay or @jasongrout decide : ) If it works without the need for setting a global variable, then it is a good option imho. |
Closing in favor of #2191. Thanks for pushing this forward, @tarzzz! @maartenbreddels - it'd be great if you could mention @tarzzz in a commit message, or rebase your PR on top of this so @tarzzz gets credit in the repo for the work done here. |
This replaces jupyter-widgets#2185 which used a global variable.
This replaces jupyter-widgets#2185 which used a global variable.
Details: #1627 (comment)