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

fix: deepcopy with structuredClone over JSON.parse/stringify #3689

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

manzt
Copy link
Contributor

@manzt manzt commented Feb 3, 2023

structuredClone is now the preferred way to deeply copy objects in JS. It is widely supported across browsers, minus some edge cases like in Safari web-workers (see the table in the linked documentation).

The old JSON.parse + JSON.stringify trick deletes any DataViews (without a custom serializer) within WidgetModel.serialize, leading to a serious bug when using raw "binary" traitlets. With the {remove,put}_buffers implementations, extra serializers are not required for transferring binary data. The following example should work without custom serializers but immediately deletes text value when this.model.save_changes() is called for the first time.

import { DOMWidgetModel, DOMWidgetView } from "@jupyter-widgets/base";
class CustomModel extends DOMWidgetModel { /* ... */ }
class CustomView extends DOMWidgetView {
  render() {
    let decoder = new TextDecoder(), encoder = new TextEncoder();
    let text = document.createElement("textarea");
    text.oninput = () => {
      let bytes = encoder.encode(text.value);
      this.model.set("text", new DataView(bytes.buffer));
      this.model.save_changes(); // internally calls JSON.parse(JSON.stringify(DataView)) -> {}
    };
    let on_change = () => text.value = decoder.decode(this.model.get("text"));
    on_change();
    this.model.on("change:text", on_change);
    this.el.appendChild(text);
  }
}
export { CustomModel, CustomView };
class CustomWidget(ipywidgets.DOMWidget):
    # ...
    text = traitlets.Any(b'hello, world').tag(sync=True)

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Binder 👈 Launch a binder notebook on branch manzt/ipywidgets/patch-1

@manzt
Copy link
Contributor Author

manzt commented Feb 3, 2023

You can see this bug with anywidget in Google Colab.

import anywidget
import traitlets

class BinaryTextArea(anywidget.AnyWidget):
    _esm = """
    let decoder = new TextDecoder(), encoder = new TextEncoder();
    export function render(view) {
        let text = document.createElement("textarea");
        text.oninput = () => {
          let bytes = encoder.encode(text.value);
          view.model.set("text", new DataView(bytes.buffer));
          view.model.save_changes();
        };
        function on_change() {
          text.value = decoder.decode(view.model.get("text"));
        }
        on_change();
        view.model.on("change:text", on_change);
        view.el.appendChild(text);
    }    
    """
    text = traitlets.Any(b'hello, world').tag(sync=True)


area = BinaryTextArea()
area

@maartenbreddels
Copy link
Member

Whow, this is good news. This makes it much better to implement binary support on the front end. I think it would be good to backport this to 7.
@jasongrout would you classify this as a fix or feature?

@jasongrout
Copy link
Member

@jasongrout would you classify this as a fix or feature?

I think it's fine to backport to 7.x. It's definitely backwards compatible, right? Especially with the fallback.

@jasongrout
Copy link
Member

The visual test failure is unrelated - looks like jlab puts a notification in the background that we are capturing:

Before:
5ce58ad328bfeb9101010a5869582d09cd9a3423

After:
e8e28c1915c46f081dfa437328e00a0e2b83efc1

Diff:
53c0f077a9d975bdaf6162ddf4a69d6885b84e5f

@martinRenou
Copy link
Member

The visual test failure is unrelated

Updating Galata seems to do the trick for getting rid of this new notification. We had the same in bqplot.

@jasongrout
Copy link
Member

Visual regression fixed in #3694. Thanks @martinRenou

@jasongrout jasongrout merged commit ab7ea4b into jupyter-widgets:main Feb 3, 2023
@jasongrout
Copy link
Member

Thanks @manzt!

@manzt manzt deleted the patch-1 branch February 3, 2023 16:51
maartenbreddels added a commit to maartenbreddels/ipywidgets that referenced this pull request Feb 6, 2023
maartenbreddels added a commit to maartenbreddels/ipywidgets that referenced this pull request Feb 6, 2023
ibdafna added a commit that referenced this pull request Sep 12, 2023
Reverts #3689 and #3738 back to the original working codebase
martinRenou added a commit to martinRenou/ipywidgets that referenced this pull request Sep 13, 2023
martinRenou added a commit to martinRenou/ipywidgets that referenced this pull request Sep 13, 2023
martinRenou added a commit to martinRenou/ipywidgets that referenced this pull request Sep 13, 2023
martinRenou added a commit that referenced this pull request Sep 13, 2023
@manzt
Copy link
Contributor Author

manzt commented Sep 30, 2023

I know this caused a lot of issues, and I'm very sorry for that. Since anywidget doesn't have the same backward compat requirements, I decided to use structuredClone there. Perhaps this is a good middle ground.

@maartenbreddels
Copy link
Member

No need to say sorry, thank you for contributing! The burden of breaking changes rests on the maintainers, we are all responsible.
I hope you continue to contribute, I love the work you are doing ❤️

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