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

feat: backward compatibility with 7.x #3358

Merged

Conversation

mariobuikhuizen
Copy link
Contributor

@mariobuikhuizen mariobuikhuizen commented Jan 26, 2022

This way widget libraries accessing JupyterPhosphorWidget and/or pWidget can run with 8.x without making a 7.x incompatible build.

For example:

This way widget libraries accessing JupyterPhosphorWidget and/or
pWidget can run with 8.x without making a 7.x incompatible build.
@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch mariobuikhuizen/ipywidgets/feat_backward_compatibility

@SylvainCorlay
Copy link
Member

Thanks! As per the discussion at the team meeting on Tuesday, we should get this in!

*/
get pWidget(): Widget {
if (!DOMWidgetView.deprecationWarningDisplayed) {
console.warn(
Copy link
Member

Choose a reason for hiding this comment

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

This is going to put a lot of warnings in consoles of users that cannot do anything about it as soon as we release. Perhaps we can start off deleting this console.warn, with just docs saying it is deprecated, then up it to a console warning later to put more pressure on migrating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention of this code was to maximally show one warning per page load. I can't see how this would happen more often, maybe I'm missing something? If one message is still too much, I'm still fine with taking it out.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant that lots of users will have this warning, not that it would show up a lot of times on a single page. I think it's gentler to give developers a chance to migrate before showing warnings to their users.

Copy link
Member

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

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

Thanks!

@jasongrout jasongrout merged commit 6456032 into jupyter-widgets:master Jan 28, 2022
mariobuikhuizen added a commit to widgetti/ipyvue that referenced this pull request Feb 2, 2022
mariobuikhuizen added a commit to widgetti/ipyvue that referenced this pull request Feb 2, 2022
@martinRenou
Copy link
Member

martinRenou commented Mar 2, 2022

@jasongrout @mariobuikhuizen Should we do an alias for processPhosphorMessage calling processLuminoMessage as well?

cc. @ibdafna

@mariobuikhuizen
Copy link
Contributor Author

Yeah, I'm for it. It just wasn't an issue in ipyvue.

@martinRenou
Copy link
Member

Thanks a lot @mariobuikhuizen ! I was thinking about it this morning, it's unclear to me how this would look like for someone who wants to still support ipywidgets<8 (they need to overwrite processPhosphorMessage) and want to support ipywidgets===8 (they need to overwrite processLuminoMessage as well).

@mariobuikhuizen
Copy link
Contributor Author

Ok, so it's used with overriding (looking at https://github.com/bqplot/bqplot/blob/dbc639836e58909250cf2ceb93aaca9e2c77d3d8/js/src/Figure.ts#L743). I don't know a solution to that off the top of my head. I will think about it for a bit.

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.

4 participants