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

Convert frontend to typescript #314

Merged
merged 10 commits into from
Sep 21, 2021
Merged

Convert frontend to typescript #314

merged 10 commits into from
Sep 21, 2021

Conversation

ianhi
Copy link
Collaborator

@ianhi ianhi commented Mar 24, 2021

  1. Switch the frontend code from js to ts
  2. Fix several small errors that typescript detected
  3. Add proper labextension:watch script to package.json
  4. Generally update the structure of the frontend to better track https://github.com/jupyter-widgets/widget-ts-cookiecutter for easier future maintainability.

I think the arguments for switching to typescript are:

  1. Most of the jupyterlab ecosystem has a preference for typescript (including widgets)
  2. Can catch errors (found some in this PR)
  3. Can be more accessible to potential contributors coming from a pure python background. (anecdotal of course but this was very much true for me)

@martinRenou what do you think?

@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch ianhi/ipympl/ts

@martinRenou
Copy link
Member

That's amazing. Thanks for doing this!

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 7, 2021

I've been stuck on this:

image

In a fresh environment if I follow the steps as in the github action then that directory exists for me - so I'm not really sure how to fix. Sometime I may have a go at just starting from the cookiecutter more fully and see if that helps.

@martinRenou
Copy link
Member

Sometime I may have a go at just starting from the cookiecutter more fully and see if that helps

Yeah. I was wondering if it was not best to do that indeed. I might have some time allocated for working on ipympl soonish, so if you don't have time and if you want I might be able to follow up on this.

@martinRenou
Copy link
Member

Although I believe your approach should work as well. I am not sure what the nbextension issue is. It might be just that the typescript compilation failed but the error didn't propagate and pip install . succeeded wrongly resulting in a broken installation. I've seen this happening once.

I can have a try.

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 7, 2021

Sometime I may have a go at just starting from the cookiecutter more fully and see if that helps

Yeah. I was wondering if it was not best to do that indeed.

My worry was that doing this may make it would make it much harder to maintain the git history

It might be just that the typescript compilation failed but the error didn't propagate and pip install . succeeded wrongly resulting in a broken installation. I've seen this happening once.

Interesting, looking through the logs that doesn't seem to have happened, but I'll try to look more closely sometime.

I might have some time allocated for working on ipympl soonish, so if you don't have time and if you want I might be able to follow up on this.

Cool, I'll keep trying of course, but not guarantees w.r.t success.

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 7, 2021

I think part of the issue was that I had accidentally deleted the ipympl/nbextension/extension.js

@@ -0,0 +1,810 @@
define("jupyter-matplotlib", ["@jupyter-widgets/base"], (__WEBPACK_EXTERNAL_MODULE__jupyter_widgets_base__) => { return /******/ (() => { // webpackBootstrap
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woops this file snuck in at some points. Will need to remove this in a rebase at some point prior to merging

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 7, 2021

Getting somewhere!
image

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 7, 2021

The LGTM for javascript should either be disabled or swapped to typescript. I'm not sure who has the rights to do that, but probably @tacaswell ?

image

@SylvainCorlay
Copy link
Member

Thank you so much @ianhi !

@ianhi
Copy link
Collaborator Author

ianhi commented Apr 7, 2021

hooorayy the tests all pass!

There are a few things I still want to clean up and #317 should probably be merged first so let's not merge just yet.

TODO:

  1. Remove the embed-bundle under docs
  2. Update readme as necessary
  3. Squash some of the commits

@ianhi
Copy link
Collaborator Author

ianhi commented Jun 4, 2021

test failures are due to the churn in jupyter_packaging which I believe is settled now:

ImportError: cannot import name 'get_version_info' from 'jupyter_packaging'

I think it's best to also convert to the method described here: https://github.com/jupyter/jupyter-packaging#as-a-build-backend

Though I admit how that works is very mysterious for me so if anyone feels inspired to have a crack at it please feel free to push here.

@martinRenou
Copy link
Member

Sorry for not looking into this earlier. I will rebase and fix the conflicts.

It works great in JupyterLab but I have an issue in Jupyter Notebook, I'll see if I can figure it out.

ianhi and others added 9 commits September 21, 2021 16:40
In the process also update some the tooling (eslint, prettier, setup.py) to more closely the widget-ts-cookiecutter.
Also fix miscellanous errors discovered by using typescript.

Finally, add a labextension watch command.

lint the correct directory

Update MANIFEST.in for TS

correctly install labextension in the test

Return to using yarn in setup.pt
@martinRenou
Copy link
Member

Rebased, it seems to work now in Jupyter Notebook as well.

Let's see what CI thinks about it.

@martinRenou
Copy link
Member

Let's goooo! Thank you a lot for this!

@martinRenou martinRenou merged commit 4f390a0 into matplotlib:master Sep 21, 2021
@ianhi ianhi deleted the ts branch September 21, 2021 15:37
@ianhi
Copy link
Collaborator Author

ianhi commented Sep 21, 2021

hooorayy! thanks for pushing it over the edge. I got it working in lab but never could figure out the notebook

@@ -6,7 +6,7 @@ define(function() {
window['requirejs'].config({
map: {
'*': {
'jupyter-matplotlib': 'nbextensions/ipympl/index',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wow that was it??? nice find

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.

None yet

3 participants