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

Stop vendoring noVNC #77

Merged
merged 9 commits into from
Feb 5, 2024
Merged

Stop vendoring noVNC #77

merged 9 commits into from
Feb 5, 2024

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Feb 3, 2024

We have been vendoring noVNC to get vnc_lite.html, maintaining a patch file so we could upgrade the version of noVNC used if needed.

noVNC publishes a JS library onnpm
that we can easily use instead, and stop vendoring the whole package! This brings us the following massive advantages:

  1. No more vendoring an entire package in!
  2. Upgrades of noVNC client become practically trivial
  3. We can change the frontend as we wish much more easily, compared to having to maintain a patch file as we do now. PRs like Add Hub/Lab/Notebook navigation buttons to VNC top bar #7 become easier.
  4. There's also some possible licensing issues around us vendoring this anyway Repository license is incorrect #27 (comment), this will get rid of that

This PR:

  • Adds a package.json to include the noVNC js package
  • Adds a webpack.config.js to provide JS bundling, so we can just ship a single viewer.js file that has everything.
  • Adds appropriate bits (stolen and adapted from jupyterhub's config) to package the built JS in the final wheel as necessary.
  • Upgrade from noVNC 1.2 to 1.4, but otherwise keep everything the same. I just copied our patched vnc_lite.html into index.html, and just split out the JS / CSS. I also changed the title of the page
  • prettier with pre-commit has done its thing on the JS, CSS and HTML. This is fine, as I am hoping we can make more changes to the UI after this.
  • Modify the Dockerfile to setup the conda env first, and then separately install the python package with pip. This makes local development much faster.
  • Add a .gitignore (which this repo was missing), with node_modules included as well as the dist/ directory with the built JS.
  • Add a test to make sure the built JS is included in the sdist and bdist
  • Add a test to make sure the built container image has the built JS

We have been vendoring noVNC to get vnc_lite.html,
maintaining a patch file so we could upgrade the version of
noVNC used if needed.

noVNC publishes a JS library [on
npm](https://www.npmjs.com/package/@novnc/novnc)
that we can easily use instead, and stop vendoring the whole
package! This brings us the following massive advantages:

1. No more vendoring an entire package in!
2. Upgrades of noVNC client become practically trivial
3. We can change the frontend as we wish much more easily,
   compared to having to maintain a patch file as we do now.
   PRs like
   #7
   become easier.

This PR:

- Adds a `package.json` to include the noVNC js package
- Adds a `webpack.config.js` to provide JS bundling, so we can just ship
  a single `viewer.js` file that has everything.
- Adds appropriate bits (stolen and adapted from jupyterhub's config)
  to package the built JS in the final wheel as necessary.
- Upgrade from noVNC 1.2 to 1.4, but otherwise keep everything the
  same. I just copied our patched `vnc_lite.html` into `index.html`,
  and just split out the JS / CSS.
- prettier with pre-commit has done its thing on the JS, CSS and
  HTML. This is fine, as I am hoping we can make more changes to the
  UI after this.
- Modify the `Dockerfile` to setup the conda env first, and then
  separately install the python package with pip. This makes local
  development *much* faster.
- Add a `.gitignore` (which this repo was missing), with
  node_modules included as well as the dist/ directory with the built
  JS.
@yuvipanda yuvipanda requested a review from manics February 3, 2024 09:42
Copy link

github-actions bot commented Feb 3, 2024

Binder 👈 Launch a binder notebook on this branch for commit a59abc4

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 823b155

Binder 👈 Launch a binder notebook on this branch for commit 68e25d0

Binder 👈 Launch a binder notebook on this branch for commit 3a6e81e

Binder 👈 Launch a binder notebook on this branch for commit 296f781

Binder 👈 Launch a binder notebook on this branch for commit 704151c

Binder 👈 Launch a binder notebook on this branch for commit 7ac5182

Binder 👈 Launch a binder notebook on this branch for commit 0ba0689

Binder 👈 Launch a binder notebook on this branch for commit cfb1a14

@yuvipanda
Copy link
Contributor Author

mmmmmmmmmmm love it
Screen Shot 2024-02-03 at 1 42 43 AM

@yuvipanda
Copy link
Contributor Author

Once this gets merged, I'll cleanup the JS file a little and implement a version of #7

@yuvipanda
Copy link
Contributor Author

https://github.com/novnc/noVNC/blob/master/docs/API.md there's also a bunch of stuff we can do to make the experience better, like automatic reconnects, sending resize events through, save screenshots, etc.

js/index.js Outdated Show resolved Hide resolved
@manics
Copy link
Member

manics commented Feb 3, 2024

The desktop fails to load on mybinder:
Loading failed for the <script> with source “https://notebooks.gesis.org/binder/jupyter/user/jupyterhub-jupy-e-desktop-proxy-rk6lnz86/desktop/dist/viewer.js”.

@yuvipanda
Copy link
Contributor Author

@manics ah interesting, it works for me locally in a container. wonder why. I'll dig through and see.

@yuvipanda
Copy link
Contributor Author

I can reproduce the failure on mybinder! Shall test.

@yuvipanda
Copy link
Contributor Author

Interesting, but it works on a JupyterHub!

@yuvipanda
Copy link
Contributor Author

(base) jovyan@jupyter-jupyterhub-2djupy-2de-2ddesktop-2dproxy-2d85ah8316:~$ ls /opt/conda/lib/python3.11/site-packages/jupyter_remote_desktop_proxy/static/
clipboard.svg  index.css  index.html

Alright, this is something about how the image is being built.

@yuvipanda
Copy link
Contributor Author

@manics it works now!

@yuvipanda
Copy link
Contributor Author

I'm going to add a quick test to make sure that the wheel and sdist have the built files too

@yuvipanda
Copy link
Contributor Author

Cleaning up some JS on top in #78

This allows for much faster iteration, by simply mounting
$(pwd) into /opt/install
Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Other than the editable install this looks good!

Since this is an application I think we should add a package.lock (and bump it with dependabot) so we can keep track of all the dependencies but that can be done in a follow-up.

Dockerfile Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

🎉🎆🥳

@manics manics merged commit e6e4afb into main Feb 5, 2024
6 checks passed
@manics manics deleted the no-vendor branch February 5, 2024 13:21
@yuvipanda
Copy link
Contributor Author

yay thank you @manics! Take a look at the other PRs too when you get a minute :D When done, I'll make a release.

@yuvipanda yuvipanda added enhancement New feature or request maintenance and removed enhancement New feature or request labels Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants