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

Update to TypeScript 4.3 #3162

Merged
merged 14 commits into from
Jun 11, 2021
Merged

Update to TypeScript 4.3 #3162

merged 14 commits into from
Jun 11, 2021

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Mar 15, 2021

Update the typescript dependency to the latest version.

Leaving as a draft for now as there might be some breaking changes to fix.

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch jtpio/ipywidgets/ts-42

@jtpio
Copy link
Member Author

jtpio commented Mar 15, 2021

One of the changes introduced in TypeScript 4.0 is causing compilation to fail with:

Error: src/widget.ts(187,14): error TS2790: The operand of a 'delete' operator must be optional.
Error: src/widget.ts(193,12): error TS2790: The operand of a 'delete' operator must be optional.
Error: src/widget.ts(760,12): error TS2790: The operand of a 'delete' operator must be optional.
Error: src/widget.ts(798,12): error TS2790: The operand of a 'delete' operator must be optional.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/#operands-for-delete-must-be-optional

Which corresponds to:

delete this.comm;

@jtpio
Copy link
Member Author

jtpio commented Mar 15, 2021

Adding to the 8.0 milestone for consideration as this might result in changes in public interfaces and typings.

@jtpio
Copy link
Member Author

jtpio commented Mar 26, 2021

One of the changes introduced in TypeScript 4.0 is causing compilation to fail with:

Another change is related to properties overriding accessors (TS2610) in 4.0:

https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/#properties-overriding-accessors-and-vice-versa-is-an-error

For example:

Error: src/widget_box.ts(139,3): error TS2610: 'pWidget' is defined as an accessor in class 'DOMWidgetView', but is overridden here in 'BoxView' as an instance property.

Link to a TypeScript playground for reference:

https://www.typescriptlang.org/play?ts=4.2.3#code/MYGwhgzhAEBCkFNoG8BQ1oBcwHMByYAtggPwBc0EmATgJYB2OA3KgL6qqiQwAiCdANwQATaAgAemBPWEx4EJGgw4EmLLgLEAFAEoU6DNGqqArtXrQA5ACMTmTAHt6llhnasgA

One convention changed here is that a widget’s _view is now always a DOMWidgetView, and disposal invalidates the typing contract (rather than having to always special-case the disposal case of a null _view).
We had to upgrade backbone to get the new preinitialize method, so we can properly initialize the tagName.
@jasongrout
Copy link
Member

I updated to TS 4.3 and fixed the errors.

@jasongrout jasongrout marked this pull request as ready for review June 9, 2021 06:33
@jasongrout
Copy link
Member

@jtpio: your commits look good. Can you review my commits, and merge if they look good?

@jasongrout
Copy link
Member

(or @vidartf or anyone else, feel free to review all of our commits and merge if they look good :)

@jasongrout jasongrout mentioned this pull request Jun 9, 2021
@jtpio
Copy link
Member Author

jtpio commented Jun 9, 2021

Thanks Jason for picking this up!

@@ -29,7 +29,7 @@
"@jupyterlab/services": "^6.0.0",
"@lumino/messaging": "^1.3.3",
"@lumino/widgets": "^1.11.1",
"backbone": "1.2.3"
"backbone": "1.4.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to double check: this backbone update was required to grab the new preinitialize method? (and then fix the TS error)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes looks like it's mentioned in the change log on https://backbonejs.org:

image

@jtpio jtpio changed the title Update to TypeScript 4.2 Update to TypeScript 4.3 Jun 10, 2021
@jtpio
Copy link
Member Author

jtpio commented Jun 10, 2021

Just checked the diff, quickly tried on Binder and it looks good 👍 Thanks!

Leaving it open one more day in case someone else also wants to have a look, and then we should be able to merge it.

@jtpio
Copy link
Member Author

jtpio commented Jun 11, 2021

Thanks!

@jtpio jtpio merged commit cd9be77 into jupyter-widgets:master Jun 11, 2021
@jtpio jtpio deleted the ts-42 branch June 11, 2021 10:55
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.

2 participants