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

Unify Component and VDOM Element Constructor Interface #755

Closed
rmorshea opened this issue May 23, 2022 · 6 comments · Fixed by #841
Closed

Unify Component and VDOM Element Constructor Interface #755

rmorshea opened this issue May 23, 2022 · 6 comments · Fixed by #841
Labels
priority-2-moderate Should be resolved on a reasonable timeline. type-revision About a change in functionality or behavior
Milestone

Comments

@rmorshea
Copy link
Collaborator

rmorshea commented May 23, 2022

Current Situation

Currently the vdom element constructor and component constructors have different interfaces. Component constructors are effectively just the same as the functions which define them. For the vdom constructor though we pass vdom(tag, attrs_dict, *children). The fact that these two constructors have different interfaces won't play well with this tagged strings PEP.

Proposed Actions

In anticipation of the work from the tagged strings PEP we should have a unified interface for constructing elements and components. I think the best way to do this would be to effectively copy the interface for React's functional components.

When constructing a component or element the interface would be:

constructor(*children, key=..., **attributes)

However the definition for the constructor would be:

def constructor(children: list[any], **attributes: any): ...

In the function definition, *children gets bundled together into a special children argument that gets passed to the function. Component functions without a children argument then do not support *children. Additionally (as is currently the case) the special key parameter is hidden and handled separately.

One wrinkle in this is that the attribute for most user defined components will be snake_case while the attributes for standard HTML elements are camelCase. I suspect this will be confusing for most users. As a result, I suggest that, for standard HTML elements, we include a snake_case to camelCase conversion. Thus something like div(background_color) would be treated the same as div(backgroundColor=...).

This will be a relatively large change, so we will provide a way for users to transition to the new interface.

Notes

We should probably do #545 at the same time. So we'd add a fallback kwarg to the constructor:

constructor(*children, key=..., fallback=..., **attributes)
@rmorshea rmorshea added flag-triage Not prioritized. priority-2-moderate Should be resolved on a reasonable timeline. and removed flag-triage Not prioritized. labels May 23, 2022
@rmorshea rmorshea added this to the 1.0 milestone May 23, 2022
@rmorshea
Copy link
Collaborator Author

@Archmonger thoughts on this?

@Archmonger
Copy link
Contributor

I would recommend automatically changing all attributes from snake_case to camelCase on the backend.

I vaguely recall seeing some regex for this in Django core.

@rmorshea
Copy link
Collaborator Author

I think I want to implement the conversion client-side since that's where they have to be interpreted, but the effect for the user would be the same - all attributes would automatically become camelCase.

@Archmonger
Copy link
Contributor

looks like there's a couple good solutions to this on the JS end as well

https://stackoverflow.com/questions/6660977/convert-hyphens-to-camel-case-camelcase
https://stackoverflow.com/questions/40710628/how-to-convert-snake-case-to-camelcase-in-my-app

@rmorshea rmorshea mentioned this issue Oct 20, 2022
3 tasks
@rmorshea
Copy link
Collaborator Author

rmorshea commented Nov 24, 2022

It looks like there's approx 200 instances I'll need to modify. I drafted a script to auto-convert the old-style to the new one, but ast.unparse removes a lot of information (e.g. visually useful new lines) so I don't think I'll use it on IDOM's source:

import ast
import re
from pathlib import Path
from idom import html

_CAMEL_CASE_SUB_PATTERN = re.compile(r"(?<!^)(?=[A-Z])")

def update_vdom_constructor_usages(file: Path) -> None:
    tree = ast.parse(file.read_text())

    changed = False
    for node in ast.walk(tree):
        if isinstance(node, ast.Call):
            match (func := node.func):
                case ast.Attribute():
                    name = func.attr
                case ast.Name(ctx=ast.Load()):
                    name = func.id
                case _:
                    name = ""
            if hasattr(html, name):
                match node.args:
                    case [ast.Dict(), *_]:
                        first_arg = node.args[0]
                        new_keywords = list(node.keywords)
                        for k, v in zip(first_arg.keys, first_arg.values):
                            if isinstance(k, ast.Constant) and isinstance(k.value, str):
                                new_keywords.append(ast.keyword(arg=conv_attr_name(k.value), value=v))
                            else:
                                break
                        else:
                            changed=True
                            node.args = node.args[1:]
                            node.keywords = new_keywords
                    case _:
                        first_arg = None

    if changed:
        tree = ast.fix_missing_locations(tree)
        file.write_text(ast.unparse(tree))

def conv_attr_name(name: str) -> str:
    return _CAMEL_CASE_SUB_PATTERN.sub("_", name).replace("-", "_").lower()

@rmorshea rmorshea mentioned this issue Nov 29, 2022
3 tasks
@rmorshea
Copy link
Collaborator Author

I have an updated script that has much less impact on code style. I've run this script against IDOM's docs and tests to automatically convert html.*() usages to the new style.

@Archmonger Archmonger added the type-revision About a change in functionality or behavior label Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-moderate Should be resolved on a reasonable 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