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 component decorator eating static type hints #1199

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JamesHutchison
Copy link

@JamesHutchison JamesHutchison commented Feb 20, 2024

By submitting this pull request you agree that all contributions to this project are made under the MIT license.

Issues

The @component decorator eats type hints so you can't easily see the parameters to your component (just shows up as ...)

Solution

This updates the type hinting so that component returns the same things its decorating

Checklist

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.

before-after-component-type-fix

The top screenshot is the current behavior. The bottom is the updated behavior. Note that the top Component is reactpy's while the bottom Component is my own.

@Archmonger
Copy link
Contributor

Archmonger commented Feb 21, 2024

It's been a while since these type hints have been broken, but if I recall they broke due to some type hinting restrictions that cropped up.

The issue was that a component should contain all of the args and kwargs of their parent (as you mention) but they also need to contain one additional kwarg key=....

At the time there was no solution, but I think the concatenate function in the newer Python typing libraries might be able to be jerry-rigged to help here?

@JamesHutchison
Copy link
Author

JamesHutchison commented Feb 21, 2024

This is... closer?

from typing import Any, Callable, Concatenate, TypeVar, ParamSpec

from reactpy.core.types import ComponentType, VdomDict

T = TypeVar("T", bound=ComponentType | VdomDict | str | None)
P = ParamSpec("P")
Q = ParamSpec("Q")


def _key_param_spec(*, key: Any | None = None) -> None:
    pass


def component(
    function: Callable[P, T], __type_helper: Callable[Q, None]=_key_param_spec
) -> Callable[Concatenate[P, Q], Component]:
    """A decorator for defining a new component.

    Parameters:
        function: The component's :meth:`reactpy.core.proto.ComponentType.render` function.
    """
    sig = inspect.signature(function)

    if "key" in sig.parameters and sig.parameters["key"].kind in (
        inspect.Parameter.KEYWORD_ONLY,
        inspect.Parameter.POSITIONAL_OR_KEYWORD,
    ):
        msg = f"Component render function {function} uses reserved parameter 'key'"
        raise TypeError(msg)

    @wraps(function)
    def constructor(*args: P.args, key: Any | None = None, **kwargs: P.kwargs) -> Component:
        return Component(function, key, args, kwargs, sig)

    return constructor

I haven't tested this well. It may look gross in PyCharm or mypy or pyright might complain. I don't have mypy set up in my project, yet.
component-refactor

On a side note, shouldn't key be a str | None?

@Archmonger
Copy link
Contributor

No, key can be any serializable value. That includes integers and some objects like UUID.

@rmorshea
Copy link
Collaborator

concatenate function in the newer Python typing libraries might be able to be jerry-rigged to help here?

Unfortunately Concatenate does not help. The PEP explicitly rejects concatenating keyword arguments.

@Archmonger
Copy link
Contributor

Archmonger commented Feb 24, 2024

Played around with this for a bit... I'm not seeing a way to fix this without changing our component interface.

Kind of upsetting that there are so many Python type hinting limitations.

There is a realistic future where pyright/mypy could infer types from an implicit return like this...

def component(function: Callable[P, T]):
    sig = inspect.signature(function)

    if "key" in sig.parameters and sig.parameters["key"].kind in (
        inspect.Parameter.KEYWORD_ONLY,
        inspect.Parameter.POSITIONAL_OR_KEYWORD,
    ):
        msg = f"Component render function {function} uses reserved parameter 'key'"
        raise TypeError(msg)

    def constructor(
        *args: P.args, key: Any | None = None, **kwargs: P.kwargs
    ) -> Component:
        return Component(function, key, args, kwargs, sig)

    return constructor

But unfortunately, seems like type checkers just give up implicit parsing as soon as key (or any new kwarg) is added to the function definition.

@JamesHutchison
Copy link
Author

What I have provides a clean view of the arguments. It doesn't include key in the argument hint, nor does it flag wrong arguments, but that feels better than just seeing ... in the IDE.
better-than-nothing

It could be stricter settings might get flagged. I know pyright used by VSCode by default doesn't flag many things.

@Archmonger
Copy link
Contributor

Mypy flags key as an invalid keyword with the latest implementation in your branch.

image

@Archmonger
Copy link
Contributor

I've created a post over at the Python typing forums to see what the sentiment is around fixing this type hinting limitation.

@Archmonger
Copy link
Contributor

Marking this as a draft since it's broken with most linters.

@Archmonger
Copy link
Contributor

If we shove key into the props dictionary {"onClick": ..., "key"=...} then this solution could move forward.

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.

None yet

3 participants