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

Raise error when instantiating invalid AnyWidget subclass #45

Merged
merged 9 commits into from
Feb 3, 2023

Conversation

tlambert03
Copy link
Collaborator

@tlambert03 tlambert03 commented Feb 1, 2023

in exploring the mechanics of this library, I was breaking all the rules 😂 and created an AnyWidget subclass without _esm or _modules. In which case you see the following javascript error:

Failed to create view for 'AnyView' from module 'anywidget' with model 'AnyModel' from module 'anywidget'
TypeError: Cannot read properties of undefined (reading 'startsWith')
    at is_href (http://localhost:8888/nbextensions/anywidget/index.js?v=20230201171813:7:14)
    at load_esm (http://localhost:8888/nbextensions/anywidget/index.js?v=20230201171813:48:7)
    at AnyView.render (http://localhost:8888/nbextensions/anywidget/index.js?v=20230201171813:76:26)
    at async http://localhost:8888/nbextensions/jupyter-js-widgets/extension.js?v=20230201171813:2:742339

Obviously: one should just "rtfd" here, but this PR provides a more explicit runtime error.

ValueError: Cannot instantiate AnyWidget without defining one of the following class attributes: ('_esm', '_module')

Since _esm is effectively an abstract class attribute, another option here (as opposed to raising in the __init__), would be to use __init_subclass__:

class AnyWidget(ipywidgets.DOMWidget):
    ...
    def __init_subclass__(cls) -> None:
        if not any(hasattr(cls, attr) for attr in _ESM_CLASS_ATTRS):
            raise ValueError(
                "AnyWidget subclass must defining one of the following "
                f"class attributes: {_ESM_CLASS_ATTRS}"
            )

the main difference there is that the immediate subclass of AnyWidget must have _esm... which could possibly restrict users making their own base classes. So this PR only errors at instantiation

@netlify
Copy link

netlify bot commented Feb 1, 2023

Deploy Preview for anywidget canceled.

Name Link
🔨 Latest commit 3c182ba
🔍 Latest deploy log https://app.netlify.com/sites/anywidget/deploys/63dc574f81ca6c0009f4f300

@manzt
Copy link
Owner

manzt commented Feb 1, 2023

Thanks for the PR :) I guess another option would be to provide a default for _esm that doesn't do anything but maybe render some HTML/link to docs. For example,

image

It's technically valid to have a widget without any frontend code I guess (the traitlets stuff will just only run on the python side without any listeners).

class MyWidget(anywidget.AnyWidget):
	value = traitlets.Int().tag(sync=True)

import ipywidgets

widget = MyWidget()
slider = ipywidgets.IntSlider()

ipywidgets.link((slider, "value"), (widget, "value"))

slider # slider will update widget.value, never need to render widget technically

The _esm isn't really essential until you try to display the class in a notebook environment (i.e., _repr_mimebundle_ is called). So either we don't let folks create a class unless _esm/_module are defined, or just show something in the front-end by default if not defined (rather than raising an obscure JS error).

@tlambert03
Copy link
Collaborator Author

Yeah I think a default no op would be good too. Can close this if you want!

@tlambert03
Copy link
Collaborator Author

tlambert03 commented Feb 2, 2023

ok, changed it to this:

Screenshot 2023-02-02 at 11 25 37 AM

(where the link is to the docs getting started page)

thoughts?

(the missing space after "attribute" is fixed)

Copy link
Owner

@manzt manzt left a comment

Choose a reason for hiding this comment

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

Looks great! Just some minor comments. I think we should add a unit test.

anywidget/widget.py Outdated Show resolved Hide resolved
anywidget/widget.py Outdated Show resolved Hide resolved
Co-authored-by: Trevor Manz <[email protected]>
@manzt manzt merged commit 1d5991d into manzt:main Feb 3, 2023
@tlambert03 tlambert03 deleted the error-invalid-subclass branch February 6, 2023 17:28
@github-actions github-actions bot mentioned this pull request Feb 13, 2023
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