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

Expose classic notebook's static assets from their original endpoints #63

Merged
merged 8 commits into from
Oct 8, 2021

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Jun 9, 2021

Classic notebook extensions (i.e. nbextensions) often reference classic notebook's static assets and templates through its static endpoint. These don't exist in jupyter_server+nbclassic. Nbclassic does exposes these items through its own endpoints and jinja template environment, but classic notebook extensions are not (and should not be) aware of nbclassic's endpoint.

Instead, this PR leverages the nbserver extension to monkey-patch JS and nbclassic and re-expose the classic notebook's assets+templates in a way that mimics the classic notebook.

This will likely fix many classic notebook extension in nbclassic.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyterlab-notebook-nbextensions/11017/6

@Zsailer
Copy link
Member Author

Zsailer commented Oct 4, 2021

Tests are failing because of a bug in jupyter_server, jupyter-server/jupyter_server#587.

Once that is merged and released, I'll restart the tests here.

@Zsailer
Copy link
Member Author

Zsailer commented Oct 8, 2021

@vidartf here's the PR I mentioned in the Jupyter Server meeting today. I'd love to get your review if you have a chance.

Specifically, here is the chunk where I have to inject handlers in a specific order in the main web application.

https://github.com/jupyterlab/nbclassic/blob/2c7cbd044209ca3390a5d8a3e817ca166a49014b/nbclassic/notebookapp.py#L245-L277

@Zsailer
Copy link
Member Author

Zsailer commented Oct 8, 2021

Fixes #46.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyter-nbextensions-configurator-not-shown/7294/9

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@Zsailer
Copy link
Member Author

Zsailer commented Oct 8, 2021

Thanks, @blink1073!

@Zsailer Zsailer merged commit eac6680 into jupyter:master Oct 8, 2021

#-----------------------------------------------------------------------------
# Get the wildcard router
router = self.serverapp.web_app.wildcard_router
Copy link
Member

Choose a reason for hiding this comment

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

Could this logic have an effect on other server extensions? Also depending on the order the extensions are registered?

For reference this seems to have affected the JupyterLab browser check: jupyterlab/jupyterlab#11350

Copy link
Member Author

Choose a reason for hiding this comment

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

Investigating this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe #77 fixes this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like there were conflicts happening between jupyter-server and notebook HTML templates, since they share the same names. This PR namespaces the Jinja template environment and monkeypatches the IPythonHandler to pull these templates from the correct environment.

@DatumWorld
Copy link

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.

5 participants