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

Simplify and fix some issues with #[component] macro #2289

Merged
merged 16 commits into from
Aug 8, 2024

Conversation

tigerros
Copy link

@tigerros tigerros commented Apr 10, 2024

  • Gets rid of a ton of code (almost half of the component.rs file + the 130 line long creatively named utils.rs file), which wasn't even being used!! This is the doc building that I introduced in 1563, and I'm sorry for introducing it. It's completely unnecessary because people can simply view the props struct through the props argument, and I don't know how it even got merged. Anyway, since then there have been some changes, and the original features made in 1563 just didn't work. I tested it out and the macro simply would not build any docs. So there was a bunch of useless code, technically being used at compilation, but not being used at runtime. Or maybe it was being used somehow, slowing down compilation but not producing any results.
  • Proper casing. I've included a little trick which will actually make the compiler prefer camel case names for the function identifiers. Previous implementations just applied #[allow(non_snake_case)] for the entire function, including the body, which would silence other stuff like variables (and also wouldn't make the compiler prefer camel case). This PR makes it so that the macro only changes case lints for the function identifier.
  • Allow struct pattern parameter parsing (e.g. fn Navbar(NavbarProps { title }: NavbarProps)). This was previously being parsed incorrectly and the macro just wouldn't compile. I noticed this was used a lot in Freya which is probably why Freya doesn't use the #[component] macro, instead opting for putting #[allow(non_snake_case)] everywhere (which, as mentioned, is incorrect because it silences warnings of other identifiers).
  • Update and generally improve the outdated docs.
  • When "inlining" props, changes the expansion to be a little cleaner. The change is very minor but it does make generated docs quite a bit cleaner (IMHO).
    #[component]
    fn Foo(bar: usize) ...
    
    // old expansion:
    fn Foo(mut __props: FooProps) ...
    // rustdoc:
    fn Foo(__props: FooProps) ...
    
    // new expansion:
    fn Foo(FooProps { bar }: FooProps) ...
    // rustdoc:
    fn Foo(_: FooProps) ...

@ealmloff
Copy link
Member

ealmloff commented Apr 11, 2024

Expanding the component name to a struct will cause lints on any components that use the component_name syntax with an underscore instead of upper camel case

@tigerros
Copy link
Author

Expanding the component name to a struct will cause lints on any components that use the component_name syntax with an underscore instead of upper camel case

That's the point, since the convention is upper camel case and not snake case.

@jkelleyrtp jkelleyrtp self-requested a review April 26, 2024 16:07
Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the fact that we're trimming the fat in this module - I was very confused by the complexity of it all last time I was digging around here.

Can we keep support for lowercase components? They're basically our unofficial syntax for wrapped web components. IE you'd wrap a stringly typed kebab-case-element {} with a strongly_typed_element {}.

We don't do anything differently with codegen there, per se, but it's a decent heuristic when glancing at code as to why those components are different.

@tigerros
Copy link
Author

Can we keep support for lowercase components? They're basically our unofficial syntax for wrapped web components. IE you'd wrap a stringly typed kebab-case-element {} with a strongly_typed_element {}.

Done. I just want to point out that they're definitely "supported", they will just trigger a warning (which you can easily suppress with #[allow(non_camel_case_types)]), not an error. I thought it made sense because camel case is the convention, but I guess I didn't consider web components.

@tigerros tigerros requested a review from jkelleyrtp April 27, 2024 19:03
@ealmloff ealmloff added core relating to the core implementation of the virtualdom labels May 14, 2024
@jkelleyrtp jkelleyrtp added the backport PRs that should be back ported to the last release label Jun 7, 2024
Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing this now, sorry for the wait.

We want to keep lowercase component support, but also allow the new deconstructed props syntax.

Also I think keeping the extra props doc is nice - it helps with discovery on optional fields.

packages/core-macro/src/component.rs Show resolved Hide resolved
packages/core-macro/src/component.rs Show resolved Hide resolved
examples/rsx_usage.rs Outdated Show resolved Hide resolved
@jkelleyrtp jkelleyrtp removed the backport PRs that should be back ported to the last release label Aug 6, 2024
@ealmloff ealmloff self-requested a review August 6, 2024 21:07
@jkelleyrtp jkelleyrtp enabled auto-merge (squash) August 8, 2024 17:32
@jkelleyrtp jkelleyrtp merged commit b97e607 into DioxusLabs:main Aug 8, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core relating to the core implementation of the virtualdom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants