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

[Feature Request] Option to let @jupyter/html-manager import js files from cdn directly #2786

Closed
Bobgy opened this issue Feb 20, 2020 · 7 comments · Fixed by #2792
Closed
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Feb 20, 2020

Current behavior is:

return requirePromise([`${moduleName}`]).catch(err => {

  1. load using requirejs first
  2. if 1. fails, load from cdn

Context

However, as I investigated in issue: kubeflow/pipelines#3114
My application is deployed in a secured cluster, it is exposed to user through a proxy that checks some cookies for authentication. However, the embedded jupyter widget visualization is run in a sandboxed iframe, so it will never be able to talk to same site urls. This caused the infinite redirection error. (the widget still loads successfully falling back to cdn, but it takes about 2 seconds to wait for the meaningless redirections.)

Feature Request

Therefore, I'm proposing this feature request. I think if we can configure behavior to load from cdn directly that will solve the problem for us.

Add an option similar to data-jupyter-widgets-cdn to skip the first try and load from cdn directly.

@jasongrout
Copy link
Member

Can you write your own loader, based on that requireLoader, and instantiate the HTML manager with that loader that does what you want? That requireLoader function is just the default, but it is meant to be overridden if you want a different behavior:

loader: (
moduleName: string,
moduleVersion: string
) => Promise<any> = requireLoader

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 20, 2020

@jasongrout Thanks for the suggestion.
I'm currently using

<script src="https://unpkg.com/@jupyter-widgets/html-manager@^0.18.0/dist/embed-amd.js" crossorigin="anonymous" data-jupyter-widgets-cdn="https://cdn.jsdelivr.net/gh/Bobgy/model-analysis@kfp/tensorflow_model_analysis/notebook/jupyter/js/dist/" crossorigin="anonymous"></script>

to load html-manager. Can you point me to where I can add the requireLoader customization?

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 20, 2020

I guess I found something close, am I expected to use the libembed-amd.js and call it as a library?

fs.writeFileSync('./dist/libembed-amd.js', output);

But I didn't find any usage example, is there any documentation out there?

@jasongrout
Copy link
Member

I didn't realize you were loading the pre-built manager (with the default loader) from a CDN, rather than using the html manager as a library in your own code. For that usecase, I think you're right that a configuration option would be the most convenient, and certainly similar to the cdn configuration option. Do you want to submit a PR?

@jasongrout jasongrout added this to the Future milestone Feb 20, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 20, 2020

Thanks, I'll see if I have time soon. Doesn't seem like a complex change to me.

@jasongrout
Copy link
Member

I think you are right that it should be pretty straightforward, especially if you follow the pattern in that file already for the other data attribute.

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 24, 2020

FYI, sent #2792 to resolve this.

@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants