Skip to content

Declare more TS typings #1672

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

Closed
3 tasks done
vidartf opened this issue Aug 24, 2017 · 18 comments
Closed
3 tasks done

Declare more TS typings #1672

vidartf opened this issue Aug 24, 2017 · 18 comments
Labels
good first issue pkg:base pkg:controls resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@vidartf
Copy link
Member

vidartf commented Aug 24, 2017

The current typescript code has a lot of anys (both explicit and implicit). These should be more explicitly typed, at least on the exported functions. A more explicit typing, maybe together with strict null checks might also reveal some bugs.

  • no explicit any typings unless absolutely necessary
  • no implicit any typings unless necessary
  • turn on strict null checking
@jasongrout
Copy link
Member

jasongrout commented Aug 24, 2017 via email

@vidartf
Copy link
Member Author

vidartf commented Aug 24, 2017

Yes, absolutely. If anybody wants to do it, that would be perfect, if not, I'll try to squeeze it in 😉

@cnishina
Copy link
Contributor

cnishina commented Oct 1, 2017

Currently working on this. 😄

@cnishina
Copy link
Contributor

cnishina commented Oct 2, 2017

Here is the examples that needs review. #1733

@vidartf
Copy link
Member Author

vidartf commented Oct 2, 2017

@cnishina I did some work previously that stranded a bit, see #1734. Always meant to come back to it, but you know how it goes. If you are able to use that as a starting point to take further that would be awesome!

@cnishina
Copy link
Contributor

cnishina commented Oct 4, 2017

@vidartf ☹️ I'm having some weird tsc errors that I did not get a few days ago. So I did an npm install in the main directory. I get the following error:

> @jupyter-widgets/[email protected] build:src /path/to/ipywidgets/packages/base
> tsc --project src

src/utils.ts(14,8): error TS2307: Cannot find module 'base64-js'.

That's weird, so I did an npm install -D @types/base64-js to add the typings from DefinitelyTyped. Then I get another error:

> @jupyter-widgets/[email protected] build:src /path/to/ipywidgets/packages/controls
> tsc --project src

src/phosphor/accordion.ts(10,8): error TS2307: Cannot find module '@phosphor/signaling'.
src/phosphor/currentselection.ts(14,8): error TS2307: Cannot find module '@phosphor/signaling'.
src/phosphor/tabpanel.ts(14,8): error TS2307: Cannot find module '@phosphor/signaling'.

Super weird. I did a brand new clone, install and came up with the same sort of errors. Is there something that I'm missing in the developer setup?

@vidartf
Copy link
Member Author

vidartf commented Oct 4, 2017

Are you using the dev-install script? If not, what happens if you follow the appropriate steps manually?

@vidartf
Copy link
Member Author

vidartf commented Oct 4, 2017

It seems a lot of this is being fixed in #1738.

@jasongrout
Copy link
Member

Yes, it's weird that somehow these problems are just now being noticed.

@cnishina
Copy link
Contributor

cnishina commented Oct 4, 2017

OOoOoOh... going to check it out.

@cnishina
Copy link
Contributor

cnishina commented Oct 5, 2017

Back on track but now I have new problems. I can add some explicit typings in so there are no implicit any's; however, some of these issues might require some additional work. For example...looking at ipywidgets/packages/controls/widget_box.ts

  • BoxView extends type DOMWidgetView
  • DOMWidgetView has property pWidget. pWidget is type Widget
  • in BoxView, the property pWidget is being set to JupyterPhosphorPanelWidget
  • JupyterPhosphorPanelWidget extends type Panel and is cannot be of type Widget

🤔

@cnishina
Copy link
Contributor

cnishina commented Oct 5, 2017

I also started to make ES6 changes to use fat arrows and let over var; however, this project appears to be set up as ES5 based on the tsconfig. In hind sight I probably should have clarified this first. Jupyter widgets is using ES5?

@jasongrout
Copy link
Member

JupyterPhosphorPanelWidget extends type Panel and is cannot be of type Widget

Panel is a subclass of Widget: https://github.com/phosphorjs/phosphor/blob/23b9d075ebc5b73ab148b6ebfc20af97f85714c4/packages/widgets/src/panel.ts#L28

@jasongrout
Copy link
Member

jasongrout commented Oct 5, 2017

Jupyter widgets is using ES5?

Jupyter widgets is using Typescript. Typescript is configured to compile arrow functions and let down to ES5 for backwards compatibility.

I was hoping to go through and mak a pass changing var to let, using arrow functions, etc. If you want to do it, go for it! (perhaps on a separate PR to make reviewing easier?)

@cnishina
Copy link
Contributor

cnishina commented Oct 5, 2017

I've been replacing that = this and using fat arrows, and let over var. I just realized my previous comment about ES5 is moot because the tsconfig is transpiling to ES5 but TypeScript does not care.

@blink1073
Copy link
Contributor

Yep, we have been treating TypeScript as ES6 + type information, not really using the experimental features like decorators.

@jasongrout
Copy link
Member

The other es6 thing I've been experimenting with is using async/await, though I try to use it sparingly since it makes the generated javascript harder to follow/debug.

@jasongrout jasongrout modified the milestones: 7.x, 7.0.x Oct 27, 2017
@vidartf
Copy link
Member Author

vidartf commented Jan 9, 2020

Fixed in #1734

@vidartf vidartf closed this as completed Jan 9, 2020
@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue pkg:base pkg:controls resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

No branches or pull requests

5 participants