Warn or prevent using unpack operator within components

[Archmonger]

trafficstars

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

https://github.com/idom-team/idom/discussions/607

[rmorshea]

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]

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]

I get a syntax error:

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

[Archmonger]

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]

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]

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]

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]

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

[Archmonger]

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),
    ]

[Archmonger]

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

[rmorshea]

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.