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

Update to jupyterlab 4 #110

Merged
merged 16 commits into from
Oct 1, 2024
Merged

Conversation

chrishavlin
Copy link
Contributor

@chrishavlin chrishavlin commented Sep 27, 2024

This PR bumps us up to jupyterlab 4.

I went through the current cookiecutter (https://github.com/jupyter-widgets/widget-ts-cookiecutter), comparing dependencies and config files and fixed a few compilation errors.

This now compiles locally in a fresh environment for me. It also seems to fix the issue i was having with the canvas viewer in firefox! I can now click and drag without issue (EDIT: maybe it doesn't... freezing again?)

I'm not positive all the changes here are necessary. will go throuhg the PR and annotate with some comments.

Also note this is based off of (and blocked by) #109

@chrishavlin chrishavlin changed the title [WIP] Update to jlab 4 [WIP] Update to jupyterlab 4 Sep 27, 2024
.eslintrc.js Outdated Show resolved Hide resolved
babel.config.js Outdated Show resolved Hide resolved
"@types/fscreen": "^1.0.1",
"@types/node": "^10.11.6",
"@types/three": "^0.141.0",
"fscreen": "^1.2.0",
"ipycanvas": "^0.8.2",
"ipycanvas": "^0.13.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to bump this to get the canvas widget to properly compile

package.json Outdated Show resolved Hide resolved
"jupyterlab~=3.1",
"hatchling>=1.21.1",
"jupyterlab>=4.0.0,<5",
"hatch-nodejs-version>=0.3.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

migration script missedhatch-nodejs-version, but the cookiecutter has it, so added it.

@@ -50,12 +50,16 @@ Tracker = "https://github.com/yt-project/widgyts/issues"

[tool.hatch.build]
artifacts = [
"widgyts/labextension/*.tgz",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want this? not sure. the cookiecutter has it.

pyproject.toml Outdated Show resolved Hide resolved
@@ -50,7 +50,7 @@ export class FullscreenButtonView extends DOMWidgetView {
this.el.classList.add('jupyter-widgets');
this.el.classList.add('jupyter-button');
this.el.classList.add('widget-button');
this.delegate('click', this.el, this._handle_click);
this.delegate('click', this.el.value, this._handle_click);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

got a compilation error without this complaining about assigning a string output to a HTMLButtonElement, based on some google/SO searching, seemed like this would work. and it does now compile. though just realized I didn't test the button, will go do that now ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm. ok, ya, i don't even see the fullscreen button any more, so i broke something here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it totally works yet. So let's not worry about this for this PR and fix it at a later time. If I can figure it out I'll update your PR.

Comment on lines +21 to +22
- name: jlpm config
run: jlpm config set -H enableImmutableInstalls false
Copy link
Contributor Author

@chrishavlin chrishavlin Sep 27, 2024

Choose a reason for hiding this comment

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

Can't work out why, but enableImmutableInstalls was set to true in the CI environment, which causes an error when there is no yarn.lock file on install:

YN0028: The lockfile would have been created by this install, which is explicitly forbidden.

but the yarn file should be created here. so setting it to false seems correct. just not sure what caused the change in the CI test enviornment in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

We may want to commit our yarn.lock file. I'm not sure if it's a universally-accepted practice for jupyterlab extensions ... but maybe I'm just out of the loop or woefully ignorant?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, i'm not sure either -- maybe we should be committing it? but I can research that separate from this PR.

.yarnrc.yml Outdated Show resolved Hide resolved
babel.config.js Outdated Show resolved Hide resolved
"@types/fscreen": "^1.0.1",
"@types/node": "^10.11.6",
"@types/three": "^0.141.0",
"fscreen": "^1.2.0",
"ipycanvas": "^0.8.2",
"ipycanvas": "^0.13.3",
"jupyter-threejs": "^2.3.0"
},
"devDependencies": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these deps were copy/pasted from the current cookiecutter, which is configured for jupyterlab 4. I did remove the jest and babel deps that are in the cookiecutter, because we're not using them anywhere (and don't need to as far as I can tell)

Copy link
Member

Choose a reason for hiding this comment

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

So there's a possibility that I added them when wasm-bundling wasn't as straightforward as it is now.

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

This works great for me. I think we should merge it.

@chrishavlin chrishavlin changed the title [WIP] Update to jupyterlab 4 Update to jupyterlab 4 Oct 1, 2024
@matthewturk matthewturk merged commit 3131303 into yt-project:main Oct 1, 2024
4 checks passed
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.

2 participants