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

Add back built-in requirejs for ipynb notebooks #132557

Closed
DonJayamanne opened this issue Sep 7, 2021 · 11 comments
Closed

Add back built-in requirejs for ipynb notebooks #132557

DonJayamanne opened this issue Sep 7, 2021 · 11 comments
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook-api verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@DonJayamanne
Copy link
Contributor

Today one could preloads scripts for an ipynb file if the controller is selected (or pre-selected when opening a notebook that was previously opened in VS Code).
However if someone opens a new ipynb notebook that was shared (i.e. opening an existing ipynb file or the first time in vscode), then VS Code will not associate any controller, as a result the pre-load scripts for kernel will not load.

The downside of this is, outputs can contains HTML that expect require.js to be available and if tha'ts not available, the output will not load. This applies to plots from popular packages such as plotly.

This will be a blocker to viewing notebooks on github.dev or even locally.

I can't think of a great usecase for such a feature request for other notebooks, this is very specific to the requirements of outputs in ipynb.

Alternatives

  • Register a custom HTML renderer for HTML (which is rendered using built in renderer). Today we alraedy have custom renderers for images, and might have to do the same for aplication/javascript as well, see here element variable not defined when rendering IPyWidgets vscode-jupyter#6054 (that's a seprate conversation, but thought I'd mention the fact that we have the need to create out own renderers for some of these simple mime types)

The downside with this is, anyone viewing HTML output in any other notebook would end up being rendered by Jupyter renderer (not the best, we've had plenty of issues where such renderers cause issues, bugs or incorrect rendering of outputs of other notebooks (one of them tracked here #125876).

@mjbvz @rebornix /cc

@DonJayamanne DonJayamanne changed the title Add ability to pre-load scripts for all ipynb files Add ability to pre-load scripts for all ipynb notebooks Sep 7, 2021
@DonJayamanne
Copy link
Contributor Author

@mjbvz Do we have an update on this issue? Noticed you referenced this issue in another PR, hence the question.

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 4, 2021

@DonJayamanne What if we added a special type of notebookRenderer that is automatically loaded before any other renderers are invoked? This would be a static contribution in the package.json

Need to come up with a proposal of what the contribution point would look like. Perhaps we can build on the extensible markdown renderers proposal, so you could write something like:

  "contributes": {
    "notebookRenderer": [
      {
        "id": "addGlobals",
        "entrypoint": { 
           "extends": "*", // Always activate before any other output renderer 
            "path": "./notebook-out/index.js",
        }
      }
    ],

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 6, 2021

Actually it need to be the reverse: a renderer wants to declare that other renderers have an implicit dependency on it. Probably should be more like:

 "contributes": {
    "notebookRenderer": [
      { 
         "type": "preload",
        "id": "addGlobals",
        "path":"./notebook-out/index.js",
      }
   ]
}

@DonJayamanne
Copy link
Contributor Author

@mjbvz Sorry i missed this.
Thanks for looking into this, I can't think of a use case for this other than for jupyter notebooks.

With this proposal, doesn't this mean this script will get loaded regardless of the mime type?

  • If yes, this seems very similar to notebook controller pre-loads
  • Hence why not combine the two? This way anyone can register pre-loads for notebooks (& we have a common API)

@rebornix rebornix added the feature-request Request for new features or functionality label Oct 11, 2021
@DonJayamanne

This comment has been minimized.

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 12, 2021

Thinking about this a bit more: are there other use cases for this API? It sounds the problem with require is jupyter specific

If that's the case, let's just bring back the logic that injects requirejs for ipynb files

@DonJayamanne
Copy link
Contributor Author

I like that approach, it's hackish, but better than adding a whole new api for now.

@rebornix rebornix removed the notebook label Oct 21, 2021
@mjbvz mjbvz changed the title Add ability to pre-load scripts for all ipynb notebooks Add back built-in requirejs for ipynb notebooks Oct 25, 2021
@mjbvz mjbvz closed this as completed in 207fece Oct 25, 2021
@mjbvz
Copy link
Collaborator

mjbvz commented Oct 25, 2021

@DonJayamanne Please give the next insiders build a test and let me know if it isn't working. It simply reverts the earlier removal of requirejs

@DonJayamanne
Copy link
Contributor Author

Sure thing, thanks for getting this in.

@mjbvz mjbvz added the verification-needed Verification of issue is requested label Oct 25, 2021
@DonJayamanne DonJayamanne added the verified Verification succeeded label Oct 26, 2021
@DonJayamanne
Copy link
Contributor Author

Looks like we're still going to need additional work in Jupyter extension.
Will discuss in the next sprint.

@rebornix rebornix removed their assignment Oct 28, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook-api verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants
@rebornix @DonJayamanne @tanhakabir @mjbvz and others