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

Exploration of hot reloading of renderers in notebooks w/ Webpack #95908

Closed
5 tasks done
connor4312 opened this issue Apr 22, 2020 · 6 comments
Closed
5 tasks done

Exploration of hot reloading of renderers in notebooks w/ Webpack #95908

connor4312 opened this issue Apr 22, 2020 · 6 comments
Assignees
Labels

Comments

@connor4312
Copy link
Member

connor4312 commented Apr 22, 2020

Documenting my brute-force path to get it working:

  1. Run your bundle with webpack dev server
    • users must set the uniqueName otherwise their bundle could fail in interesting ways if other renderers use Webpack as well
    • users need to allow the null host. Not a big deal, can be given in the template
    • the default webpack sourcemap mode doesn't work, need to explicitly set e.g. inline-source-map
  2. Import the bundle script from the URL.
    • currently this.preloads doesn't support loading http urls. Just put the <script> tag directly in my render output for now
  3. Webpack hot reloading refreshes the entire page
    • enable hot reloading
      • hot reloading requires code changes to work ⚠️
      • hot reloading requests the manifest from a relative path. Users need to set the publicPath in the webpack config to the dev server public path
      • it seems like allowing the host null doesn't work for the "hot" manifest (even though that allowed the websocket to initially connect). Need to just disableHostCheck instead? Nope, seems we need to manually set headers: CORS issue with "webpack-dev-server": "^1.14.1" webpack/webpack-dev-server#533 (comment)
      • then it works! As long as the entrypoint doesn't change

Here's the resulting code:


Outcomes/questions:

  1. ✋ Can we make preloads accept HTTP URLs? It always changes the scheme to vscode-resource, e.g. vscode-resource://localhost:8116/out/client.bundle.js Allow preloading http resources from localhost in notebooks #95988
  2. 👀 How can we gracefully make "debug mode" of notebook extensions pull bundles from the web server versus the filesystem in the compiled version? Tell extensions whether they're running in development mode #95926
  3. We should make sure all these Webpack configuration bits are set in the notebook starter/template project. Build initial renderer starter code sample/repo #95989
  4. Should we provide a standard API in notebooks, or an npm module, that implements the "bootstrap" code I linked above? Or maybe being in the starter is fine. Part of the starter code
  5. If there is an event that causes the webview to reload (such as a declined HMR dependency) we should rerender all cells that were previously rendered. Right now the views just get cleared out. Rerender cell output in the viewport if the notebook renderer webview reloads #95990
@connor4312 connor4312 self-assigned this Apr 22, 2020
@connor4312
Copy link
Member Author

/cc @rebornix

@rebornix
Copy link
Member

rebornix commented Apr 22, 2020

@connor4312 really good work!

  1. Can we make preloads accept HTTP URLs? It always changes the scheme to vscode-resource, e.g. vscode-resource://localhost:8116/out/client.bundle.js

Currently the code tries to restrict the resource to be local ones, thus it rewrites the scheme. To support https, we can bypass https scheme pre loads.

  1. How can we gracefully make "debug mode" of notebook extensions pull bundles from the web server versus the filesystem in the compiled version?

As we discussed offline, something like context.inDevelopment can work.

  1. Should we provide a standard API in notebooks, or an npm module, that implements the "bootstrap" code I linked above? Or maybe being in the starter is fine.

I prefer scaffold/starter code than npm packages which hide the details. My feeling is extension authors need to understand what's going on under the hood at the end once they introduce more libraries/packages into the web pack config.

@rebornix
Copy link
Member

  1. If there is an event that causes the webview to reload (such as a declined HMR dependency) we should rerender all cells that were previously rendered. Right now the views just get cleared out.

This one is interesting. When the webview is refreshed, we need to render cell outputs again which are currently visible. Not sure yet if we have a hook in WebView or not. If there is one we should listen to it and trigger a viewport re-render.

@connor4312
Copy link
Member Author

connor4312 commented Apr 22, 2020

Currently the code tries to restrict the resource to be local ones, thus it rewrites the scheme. To support https, we can bypass https scheme pre loads.

webpack is http by default; getting and configuring trusted self-signed certs is burdensome. Maybe we can allow loading http resources from localhost?

As we discussed offline, something like context.inDevelopment can work.

Issue linked :)

I prefer scaffold/starter code than npm packages which hide the details. My feeling is extension authors need to understand what's going on under the hood at the end once they introduce more libraries/packages into the web pack config.

Makes sense. I can, once we stabilize a bit more 🙂, add a Notebook renderer as an option in vscode-generator-code (or maybe we should have a single "notebook" option which the user can pick whether it should contain a renderer and/or provider). Using the generator also gives us the ability to set a uniqueName at generation time. Tomorrow morning I'll create a small repo to contain our 'ideal' renderer starter code, and once we're happy with it I'll add it to the generators.

If there is one we should listen to it and trigger a viewport re-render.

👍

@rebornix
Copy link
Member

rebornix commented Apr 22, 2020

webpack is http by default; getting and configuring trusted self-signed certs is burdensome. Maybe we can allow loading http resources from localhost?

Is allowing http from localhost only in development mode enough?

add a Notebook renderer as an option in vscode-generator-code (or maybe we should have a single "notebook" option which the user can pick whether it should contain a renderer and/or provider)

👍 we can add notebook option and show Provider, Executor, Renderers as secondary options.

@connor4312
Copy link
Member Author

Yea I think that should be fine

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants