-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add the ability to use non-literal string as attribute names #2593
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
Conversation
e9d9da7
to
5b5d00c
Compare
Visit the preview URL for this PR (updated for commit 6139274): https://yew-rs-api--pr2593-dynamic-attr-name-fy7rmfls.web.app (expires Tue, 19 Apr 2022 18:01:01 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
|
Can you investigate the hit on bundle-size across the board? Seems unexpected and unintended to me, is the optimizer getting confused? |
I tried to investigate but I couldn't find anything directly related to this PR. Here's the twiggy output:
If you know of a solution, I would like to hear it. |
5b5d00c
to
9f98b7f
Compare
I tried to reduce the bundle size as much as I could. I don't have any other leads on what I can do to try to reduce the bundle size |
I'll also have a go at this before adding my review. On a superficial look, lgtm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a way to keep the size small. Other than that, looks good to me. We might want to revisit the implementation at some point, it seems to collect()
, i.e. recreate, IndexMap
a few times, which sounds unfortunate for performance.
@@ -57,6 +58,7 @@ pub enum AttrValue { | |||
impl Deref for AttrValue { | |||
type Target = str; | |||
|
|||
#[inline(always)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think theses inline
annotations actually effect anything, might as well remove them again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just keep them as they hint the compiler. The stdlib also has them
Description
Use AttrValue instead of &'static str in Attributes::IndexMap
Fixes #2583
Checklist
cargo make pr-flow