Convert frontend to typescript by ianhi · Pull Request #314 · matplotlib/ipympl · GitHub
Skip to content

Convert frontend to typescript - #314

Merged
martinRenou merged 10 commits into
matplotlib:masterfrom
ianhi:ts
Sep 21, 2021
Merged

Convert frontend to typescript#314
martinRenou merged 10 commits into
matplotlib:masterfrom
ianhi:ts

Conversation

@ianhi

@ianhi ianhi commented Mar 24, 2021

Copy link
Copy Markdown
Collaborator
  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

github-actions Bot commented Mar 24, 2021

Copy link
Copy Markdown
Contributor

@martinRenou

martinRenou commented Mar 24, 2021

Copy link
Copy Markdown
Member

That's amazing. Thanks for doing this!

@ianhi

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator Author

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

martinRenou commented Apr 7, 2021

Copy link
Copy Markdown
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

martinRenou commented Apr 7, 2021

Copy link
Copy Markdown
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

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator Author 
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

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator Author

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

ianhi
ianhi commented Apr 7, 2021
View reviewed changes
Comment thread docs/source/_static/embed-bundle.js Outdated
@@ -0,0 +1,810 @@
define("jupyter-matplotlib", ["@jupyter-widgets/base"], (__WEBPACK_EXTERNAL_MODULE__jupyter_widgets_base__) => { return /******/ (() => { // webpackBootstrap

@ianhi ianhi Apr 7, 2021

Copy link
Copy Markdown
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

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator Author

Getting somewhere!
image

@ianhi ianhi mentioned this pull request Apr 7, 2021
Merged
@ianhi

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator Author

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

SylvainCorlay commented Apr 7, 2021

Copy link
Copy Markdown
Member

Thank you so much @ianhi !

@ianhi

ianhi commented Apr 7, 2021

Copy link
Copy Markdown
Collaborator Author

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
ianhi force-pushed the ts branch 2 times, most recently from 39f6c4b to 2b1b389 Compare April 8, 2021 03:39
@ianhi

ianhi commented Jun 4, 2021

Copy link
Copy Markdown
Collaborator Author

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

martinRenou commented Sep 21, 2021

Copy link
Copy Markdown
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
@ianhi @martinRenou
rename to .ts
f177df3
@ianhi @martinRenou
move js-> src
dc59c1f
@ianhi @martinRenou
extend gitignore for typescript
fb2605c
@ianhi @martinRenou
Convert frontend to typescript
eff7153 
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
@ianhi @martinRenou
clean up issues with gitignore
b541997
@ianhi @martinRenou
Use built wheel for the tests
c17a0dc
@ianhi @martinRenou
run prettier
522f5e8
@ianhi @martinRenou
Update readme for typescript
1c582de
@martinRenou
Fix Jupyter Notebook
8e0538b
@martinRenou

martinRenou commented Sep 21, 2021

Copy link
Copy Markdown
Member

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

Let's see what CI thinks about it.

@martinRenou

martinRenou commented Sep 21, 2021

Copy link
Copy Markdown
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

ianhi commented Sep 21, 2021

Copy link
Copy Markdown
Collaborator Author

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

ianhi
ianhi commented Sep 21, 2021
View reviewed changes
Comment thread ipympl/nbextension/extension.js Outdated

@ianhi ianhi Sep 21, 2021

Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ianhi @martinRenou @SylvainCorlay