Skip to content

JupyterLab 2 support #38

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

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Mar 18, 2020

When installing this extension on JupyterLab 2 I noted the following warning.

WARNING in @jupyter-widgets/base
      Multiple versions of @jupyter-widgets/base found:
        1.2.5 ./~/@jupyter-widgets/jupyterlab-sidecar/~/@jupyter-widgets/base from ./~/@jupyter-widgets/jupyterlab-sidecar/~/@jupyter-widgets/base/lib/index.js
        3.0.0 ./~/@jupyter-widgets/jupyterlab-manager/~/@jupyter-widgets/base from ./~/@jupyter-widgets/jupyterlab-manager/~/@jupyter-widgets/base/css/index.css

This PR attempts to fix support for JupyterLab 2, but my knowledge of JupyterLab and TypeScript is lacking to complete this PR. I hope that someone else can complete it!

What I've done

  • Updated all dependencies in package.json
  • Added "esModuleInterop": true, in tsconfig.json to workaround an typescript compile error that probably can be resolved in a better manner.
  • Added @lumino/coreutils as a dependency, as @phosphor/coreutils was used as a dependency but not specified as one in plugin.ts.
  • Got help from @jasongrout to resolve some typescript compilation issues
  • Resolved one issue with the npm run test:chrome test, but it crash later with another issue. But...

What I have not done

I've not fixed the tests entirely, I still crash with ReferenceError: regeneratorRuntime is not defined after my browser starts. I assume it is because these tests never have been configured to run successfully before (#19). As the tests are unrelated to this PR, and I failed to fix them in 1 hour, I'll settle now and suggest this is ready for merge.

@consideRatio
Copy link
Contributor Author

@jasongrout, this extension have a dependency on jupyterlab-manager, is that a mistake? I found the following references in the code base.

// TODO: import from @jupyter-widgets/jupyterlab-manager once Output is
// exported by the main module.
import {
OutputModel
} from '@jupyter-widgets/jupyterlab-manager/lib/output';

import {
output
} from '@jupyter-widgets/jupyterlab-manager';

@jasongrout
Copy link
Member

@jasongrout, this extension have a dependency on jupyterlab-manager, is that a mistake? I found the following references in the code base.

The sidecar explicitly uses jupyterlab-specific things, so yes, it is appropriate to have a dependency. That widget only works in jlab.

Most widgets are not only for jlab, and probably should not depend on the jlab widget manager.

The failing test was probably coming from a cookiecutter, and we needed
to adjust it to the actual code we implemented.
@consideRatio consideRatio changed the title Initial steps for JupyterLab 2 support JupyterLab 2 support Mar 20, 2020
@kylebarron
Copy link

What else is needed to move this PR forward?

@jasongrout
Copy link
Member

It looks like we just need someone to test it, merge it, and release it. Can you test it?

@kylebarron
Copy link

I might be able to find some time this weekend.

Are you also looking for new test cases in the PR, or just to, e.g. test ipyleaflet on this branch?

@jasongrout
Copy link
Member

Are you also looking for new test cases in the PR, or just to, e.g. test ipyleaflet on this branch?

I'm not looking for new test cases. I think testing ipyleaflet on this branch should be fine.

@bernhard-42
Copy link

I also tried to fix it based on this PR.
It compiles, it works in the browser (nice) and the karma test run - however they fail.
Don't know why up to now

@bernhard-42
Copy link

btw. this is where both tests fail:

  Sidecar
    SidecarModel
16 04 2020 20:47:01.047:WARN [reporter]: SourceMap position not found for trace:     at SidecarModel.initialize (base/src/widget.js?61103a732e53ec236c6426e01399c25f38578cd9:5:1146)
16 04 2020 20:47:01.047:WARN [reporter]: SourceMap position not found for trace:     at new SidecarModel (base/src/widget.js?61103a732e53ec236c6426e01399c25f38578cd9:5:465)
      ✖ should be createable
        Chrome 80.0.3987 (Mac OS X 10.15.4)
      TypeError: Cannot read property 'sessionContext' of undefined
          at SidecarModel.initialize (/var/folders/6f/_pgkb3vj1637f9h4whkg1_200000gn/T/karma-typescript-bundle-917476LFXqTwYv1g2.js:134944:35)
          at SidecarModel.initialize (src/widget.js:5:1146)

@bernhard-42
Copy link

Looks like the DummyManager needs to be extended to have a mock Context (the access to widget_manager.context.sessionContext.kernelChanged.connect fails)
This goes beyond my understanding of the jupyterlab internals, happily leaving the field to others who know more about it ...

@jasongrout
Copy link
Member

Thanks for tracking that down. @vidartf - have you happened to address this in your opinionated ts cookiecutter?

@jasongrout
Copy link
Member

On the other hand, I agree with @consideRatio:

I've not fixed the tests entirely, I still crash with ReferenceError: regeneratorRuntime is not defined after my browser starts. I assume it is because these tests never have been configured to run successfully before (#19). As the tests are unrelated to this PR, and I failed to fix them in 1 hour, I'll settle now and suggest this is ready for merge.

The tests failing was already an issue, and I think can be solved on another issue. You tested this PR and it was working in jlab 2. Thanks!

@jasongrout jasongrout merged commit 1df3a5e into jupyter-widgets:master Apr 16, 2020
@jasongrout
Copy link
Member

Thanks everyone. I just released sidecar version version 0.4.0 and it works in jlab 2.

@bernhard-42
Copy link

@jasongrout @consideRatio For the records, I could solve the regenerator error by adding regenerator-runtime and core-js to dev dependencies and including node_modules/regenerator-runtime/runtime.js into the karma config, see bernhard-42@1550977
But then the tests fail with the above sessionContext error

Maybe this helps in the future to get the tests running again

@jasongrout
Copy link
Member

Would you mind submitting a PR with your changes? I had a difficult time getting the tests even to the point of running and getting any sort of error. Once I can get things to having that sessionContext error, it will be much easier to fix things.

@vidartf
Copy link
Member

vidartf commented Apr 17, 2020

@vidartf - have you happened to address this in your opinionated ts cookiecutter?

I don't think so, as most widgets don't interact directly with the session/kernel.

@bernhard-42
Copy link

@jasongrout created #47.
Hope it is ok that it is not rebased to your merge yesterday ...

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