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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tigerros
Copy link
Contributor

@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
Contributor 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
Contributor 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.

@ealmloff ealmloff added core relating to the core implementation of the virtualdom labels May 14, 2024
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.

None yet

3 participants