reactive-python / reactpy

It's React, but in Python
https://reactpy.dev
MIT License
7.89k stars 317 forks source link

Fix component decorator eating static type hints #1199

Closed JamesHutchison closed 3 weeks ago

JamesHutchison commented 9 months ago

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

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

rmorshea commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

image

Archmonger commented 9 months ago

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

Archmonger commented 8 months ago

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

Archmonger commented 5 months ago

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

Archmonger commented 3 weeks ago

Closing in favor of https://github.com/reactive-python/reactpy/issues/993 which will need to be drafted in the future.