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

Warn or prevent using unpack operator within components #615

Open
Archmonger opened this issue Jan 25, 2022 · 11 comments
Open

Warn or prevent using unpack operator within components #615

Archmonger opened this issue Jan 25, 2022 · 11 comments
Labels
priority-3-low May be resolved one any timeline. type-revision About a change in functionality or behavior

Comments

@Archmonger
Copy link
Contributor

Archmonger commented Jan 25, 2022

Using the unpack operator is dangerous within a component's return. ReactPy relies on the position of child elements to keep track of hooks (for any element that doesn't have a key=... defined). We'll either need to warn against using it, or completely prevent it from being used.

There is currently has no way of providing "key does not exist for dynamic children" warnings when using the unpack operator.

Original Discussion

#607

@ghost ghost assigned rmorshea Jan 25, 2022
@rmorshea
Copy link
Collaborator

The challenge here is in figuring out when to warn. Not all usages of *args in a component are invalid. For example,

@idom.component
def Example():
    do_something(*args)  # ok
    return html.div(*children)  # no ok

One could use the heuristic that anything in the return statement must avoid unpacking but that has further complications:

@idom.component
def Example():
    an_element = html.div(*some_children)  # not ok
    return html.div(
        an_element,
        make_an_element(*args)  # ok if `args` is not a list of children
    )

My worry here is that one of the following things happens:

  1. We don't warn enough, in which case the user gets a false sense of security by turning this check on.
  2. We warn too much, in which case users may just turn off this check entirely.

I've been tempted to use the div[...] syntax because div[*args] is invalid syntax. Thus, it's impossible to do the wrong thing.

@Archmonger
Copy link
Contributor Author

But isn't div[*(args)] valid syntax? I utilize that all over the Django parts of Conreq. So in my case I would have never received a warning.

@rmorshea
Copy link
Collaborator

I get a syntax error:

>>> div[*(args)]
  File "<stdin>", line 1
    div[*(args)]
               ^
SyntaxError: invalid syntax

@Archmonger
Copy link
Contributor Author

You're right. It's only during the initialization of an array where I was using the unpack operator.

Wouldn't the syntax you're suggesting make constructing elements awkward? I'm envisioning something like....

div[0] = Element1
div[1] = Element2

Is it even possible to use the index operator to denote a list of elements like the previous syntax?

div[ Element1, Element2 ]

@rmorshea
Copy link
Collaborator

The following seems to work:

>>> class X:
...     def __getitem__(self, item):
...         return item
... 
>>> x = X()
>>> x[1, 2, 3, 4, 5, 6, 7, 8, 9]
(1, 2, 3, 4, 5, 6, 7, 8, 9)

@Archmonger
Copy link
Contributor Author

That's a pretty clever way to solve this one, I say lets do it.

Perhaps v1->v2 target? Unless it's a super quick implementation.

@rmorshea
Copy link
Collaborator

Quick to implement if a deprecation warning is included until the old syntax is dropped.

I worry about the syntax being off-putting though.

Technically React also has a similar issue with the Javascript spread operator if you're not using JSX syntax. Though, everyone uses JSX so it doesn't really come up.

@rmorshea
Copy link
Collaborator

I guess the whole framework is probably odd to Python devs though, so maybe this isn't so bad.

@Archmonger
Copy link
Contributor Author

Archmonger commented Jan 25, 2022

I guess the whole framework is probably odd to Python devs though, so maybe this isn't so bad.

Agreed. IDOM is pretty far removed from Python styling so it doesn't really matter how much you continue to deviate at this point. I generally take the stance that frameworks should protect developers from ever writing bad code (when possible).

Visually, the styling doesn't look "ugly"

# Old
def homepage(websocket):
    state, set_state = idom.hooks.use_state(HomepageState())

    return div(
        navbar(websocket, state, set_state),
        modal(websocket, state, set_state),
        sidebar(websocket, state, set_state),
        viewport_loading(websocket, state, set_state),
        viewport_primary(websocket, state, set_state),
        viewport_secondary(websocket, state, set_state),
    )
# New
def homepage(websocket):
    state, set_state = idom.hooks.use_state(HomepageState())

    return div[
        navbar(websocket, state, set_state),
        modal(websocket, state, set_state),
        sidebar(websocket, state, set_state),
        viewport_loading(websocket, state, set_state),
        viewport_primary(websocket, state, set_state),
        viewport_secondary(websocket, state, set_state),
    ]

@rmorshea rmorshea transferred this issue from reactive-python/reactpy-flake8 Jan 25, 2022
@rmorshea rmorshea added priority-2-moderate Should be resolved on a reasonable timeline. type-revision About a change in functionality or behavior labels Jan 25, 2022
@rmorshea rmorshea added this to the 1.0 milestone Jan 25, 2022
@Archmonger Archmonger changed the title Warn against using unpack operator within components Warn or prevent using unpack operator within components Jan 25, 2022
@Archmonger
Copy link
Contributor Author

I still want to note, it is still viable to not develop this and just appropriately document the limitation.

@rmorshea
Copy link
Collaborator

Ok, here's what I think we should do here:

  1. clearly document this.
  2. see how commonly people report problems with this.
  3. based on the commonality of the problem consider making the syntax change as part of a 2.0 release.

@rmorshea rmorshea modified the milestones: 1.0, 2.0 Jan 26, 2022
@rmorshea rmorshea added priority-3-low May be resolved one any timeline. and removed priority-2-moderate Should be resolved on a reasonable timeline. labels Jan 26, 2022
@rmorshea rmorshea removed their assignment Jul 1, 2022
@rmorshea rmorshea mentioned this issue Dec 1, 2022
3 tasks
@Archmonger Archmonger modified the milestones: Luxury, Essential Jan 29, 2023
@rmorshea rmorshea removed this from the Essential milestone Feb 21, 2023
Archmonger referenced this issue in williamneto/reactpy-material Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low May be resolved one any timeline. type-revision About a change in functionality or behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants