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

WidgetManagerBase: Improve create_view return type #2662

Merged

Conversation

martinRenou
Copy link
Member

Given a video widget model VideoModel which has an associated view VideoView. The following code does not compile:

createVideo(videoModel: VideoModel) {
    this.create_child_view(videoModel).then((videoView: VideoView) => {
        // Do something
    });
}

With the following error:

Argument of type '(videoView: VideoView) => VideoView' is not assignable to parameter of type '(value: DOMWidgetView) => VideoView | PromiseLike<VideoView>'.
  Types of parameters 'videoView' and 'value' are incompatible.
    Type 'DOMWidgetView' is not assignable to type 'VideoView'.

This PR allows doing:

createVideo(videoModel: VideoModel) {
    this.create_child_view<VideoView>(videoModel).then((videoView: VideoView) => {
        // Do something
    });
}

@pbugnion
Copy link
Member

pbugnion commented Jan 8, 2020

Is this still relevant given the discussion on removing createView this morning? Is there still value in merging this in the interim?

@jasongrout
Copy link
Member

We're removing display_model/display_view in favor of keeping and using create_view.

@jasongrout jasongrout added this to the 8.0 milestone Jan 8, 2020
@martinRenou martinRenou force-pushed the create_view_return_type branch 2 times, most recently from 092660e to 1f4262b Compare January 10, 2020 09:21
@martinRenou
Copy link
Member Author

@jasongrout would it be fine to merge this PR?

options = { parent: this, ...options};
return this.model.widget_manager.create_view(child_model, options)
return this.model.widget_manager.create_view<VT>(child_model, options)
Copy link
Member

Choose a reason for hiding this comment

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

I think once you have the overrides, you won't need to specify the type parameter here, as it should select the right override.

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! Thanks you

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it does not seem to compile without it:

src/widget.ts:696:9 - error TS2322: Type 'Promise<DOMWidgetView>' is not assignable to type 'Promise<VT>'.
  Type 'DOMWidgetView' is not assignable to type 'VT'.
    'DOMWidgetView' is assignable to the constraint of type 'VT', but 'VT' could be instantiated with a different subtype of constraint 'WidgetView'.

696         return this.model.widget_manager.create_view(child_model, options)
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
697             .catch(utils.reject('Could not create child view', true));

I feel like this compilation error is not legit though, as DOMWidgetView is a subclass of WidgetView, what do you think?

@martinRenou martinRenou force-pushed the create_view_return_type branch 5 times, most recently from 4ec05d9 to 65346ed Compare January 10, 2020 15:34
Signed-off-by: martinRenou <martin.renou@gmail.com>
@martinRenou martinRenou force-pushed the create_view_return_type branch from 378c679 to 480f1bc Compare January 10, 2020 15:59
@jasongrout jasongrout merged commit c6f5939 into jupyter-widgets:master Jan 10, 2020
@martinRenou martinRenou deleted the create_view_return_type branch January 10, 2020 16:43
@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
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants