search

jupyter-widgets / jupyterlab-sidecar

A sidecar output widget for JupyterLab
BSD 3-Clause "New" or "Revised" License
247 stars 39 forks source link

JupyterLab 2 support #38

Closed consideRatio closed 4 years ago

consideRatio commented 4 years ago

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

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 commented 4 years ago

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

https://github.com/jupyter-widgets/jupyterlab-sidecar/blob/720747a7b022343029ae75129337510576500721/src/widget.ts#L4-L8

https://github.com/jupyter-widgets/jupyterlab-sidecar/blob/720747a7b022343029ae75129337510576500721/src/plugin.ts#L16-L18

jasongrout commented 4 years ago

@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.

kylebarron commented 4 years ago

What else is needed to move this PR forward?

jasongrout commented 4 years ago

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

kylebarron commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jasongrout commented 4 years ago

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 commented 4 years ago

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

bernhard-42 commented 4 years ago

@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 https://github.com/bernhard-42/jupyterlab-sidecar/commit/1550977900a7a0e06e061b8da9d13b5ed5c17bee But then the tests fail with the above sessionContext error

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

jasongrout commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@jasongrout created https://github.com/jupyter-widgets/jupyterlab-sidecar/pull/47. Hope it is ok that it is not rebased to your merge yesterday ...