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

Add migration guide for 8.0 #3405

Merged
merged 1 commit into from
May 10, 2022

Conversation

martinRenou
Copy link
Member

No description provided.

@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Binder 👈 Launch a binder notebook on branch martinRenou/ipywidgets/add_migration_guide

@jasongrout jasongrout added this to the 8.0 milestone Mar 3, 2022
@martinRenou martinRenou force-pushed the add_migration_guide branch 2 times, most recently from 3b571d0 to 3160041 Compare March 3, 2022 14:27
Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

I've made a phosphor/lumino message processing compatibility implementation in a package like this. Any input on the implementation itself? Any suggestions for where to best put it in the migration guide?

class MyWidget extends DOMWidgetView {

  _processLuminoMessage(msg: Message, _super: (msg: Message) => void): void {
    _super.call(this, msg);
    switch (msg.type) {
    case 'after-attach':
      // Auto-sizing should be updated when attached to DOM:
      this.onChange();
      break;
    }
  }

  processPhosphorMessage(msg: Message): void {
    this._processLuminoMessage(msg, (DOMWidgetView as any).processPhosphorMessage);
  }

  processLuminoMessage(msg: Message): void {
    this._processLuminoMessage(msg, (DOMWidgetView as any).processLuminoMessage);
  }
}

docs/source/migration_guides.md Outdated Show resolved Hide resolved
docs/source/migration_guides.md Outdated Show resolved Hide resolved
@vidartf
Copy link
Member

vidartf commented Mar 15, 2022

We should make sure we include some notes about what we recommend in terms of depending on phosphor / lumino . This is probably important in terms of which version of /base can be used? It is also important in terms of allowing JupyterLab to drop the resolution key in the future.

@martinRenou
Copy link
Member Author

martinRenou commented Apr 14, 2022

cc. @mariobuikhuizen @maartenbreddels

We discussed Phosphor/Lumino (and ipywidgets 7/8) compatibility with @ibdafna yesterday. Some open questions raised:

  • feat: backward compatibility with 7.x #3358 was introducing some aliases that allows to stay compatible with ipywidgets 8 for free, but it is missing some aliases, like JupyterLuminoPanelWidget. Should we fix that?
  • What @vidartf is suggesting in Add migration guide for 8.0 #3405 (review) sounds good to me, but it requires people to update their code, so what would be the point of adding aliases (as suggested above) anyway? So my question is should we get rid of the aliases and just document how to update user code (what @vidartf is suggesting + document the different aliases to add)?
    I cannot think of a way to fix the processPhosphorMessage/processLuminoMessage incompatibility without changing user code.

Sorry I didn't join the widgets meeting for some time, I'll join the next one.

@jasongrout
Copy link
Member

Here is another example of a migration to 8.0, which was only complicated by the fact that it tried to use display_model in an unconventional way: jupyter-widgets/jupyterlab-sidecar#86

@jasongrout
Copy link
Member

jasongrout commented Apr 16, 2022

ipyleaflet also seems very easy to upgrade: jupyter-widgets/ipyleaflet#968

Edit: Oh, it also needs the processPhosphorMessage shim: see jupyter-widgets/ipyleaflet#969

@jasongrout-db
Copy link

We may need to put the message compatibility shim in the base widget package itself so that widget packages will inherit it and be able to work under both 7 and 8. Then the widget packages will be able to work under both an ipywidgets 7 manager (which calls processPhosphorMessage) and an ipywidgets 8 manager (which calls processLuminoMessage).

@martinRenou martinRenou force-pushed the add_migration_guide branch 7 times, most recently from 3a1449d to 149fcd9 Compare April 25, 2022 09:59
@martinRenou
Copy link
Member Author

@vidartf @jasongrout This should be good for another review, thanks for your comments!

@vidartf vidartf merged commit 2de6e84 into jupyter-widgets:master May 10, 2022
@martinRenou martinRenou deleted the add_migration_guide branch May 10, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants