-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Introduce immutable string, array and map #2563
Conversation
Visit the preview URL for this PR (updated for commit 1e4b52a): https://yew-rs--pr2563-immutable-gv5uzv7v.web.app (expires Fri, 01 Jul 2022 14:51:15 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
👋 @hamza1311 can I have your feedback on this before I complete the documentation and publish |
Size Comparison
✅ None of the examples has changed their size significantly. |
This looks fine for the most part. |
Actually that's not good. This is why in IArray, for instance, there is a requirement that T implements ImplicitClone so when you call
That's a Rust problem in general. But I think this is a special case. Usually it is solved by wrapping the type in a container and implementing Deref. But I can also imagine adding features to |
This macro looks nice btw. It emulates the deconstructor of JS: #[test]
fn imap_deconstruct() {
let my_imap = [(IString::from("foo"), 1), (IString::from("bar"), 2)]
.into_iter()
.collect::<IMap<IString, u32>>();
imap_deconstruct!(
let { foo, bar, baz } = my_imap;
let { foobarbaz } = my_imap;
);
assert_eq!(foo, Some(1));
assert_eq!(bar, Some(2));
assert_eq!(baz, None);
assert_eq!(foobarbaz, None);
} |
I can't help but feel like we are going in the opposite direction of what Rust tells us to do |
That's because it is! Glad you see it too. Props are propagated from parents to child, you can't pass reference in props of components because the children are updated later. You need garbage collection to propagate props or you'll be cloning a lot of memory. That's why we put those Rc everywhere. |
@WorldSEnder done 👍 all tests are green |
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.
This can be merged after #2740 because otherwise CI breaks
@@ -29,6 +29,7 @@ thiserror = "1.0" | |||
|
|||
futures = { version = "0.3", optional = true } | |||
html-escape = { version = "0.2.9", optional = true } | |||
implicit-clone = { version = "0.2", features = ["map"] } |
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.
Do we need to enable map
feature? I don't think we use it.
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.
No... I added it because there was the re-export and it felt like a core feature. But on the other hand, it's an invitation to use an anti-pattern that is not even useful for Yew. It's maybe best to remove it entirely from implicit-clone too
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 agree that we should remove it:
implicit-clone = { version = "0.2", features = ["map"] } | |
implicit-clone = { version = "0.2" } |
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.
What do you think about removing it from implicit-clone too? You're also maintaining a UI framework so I guess you probably met a lot of use cases where we need to pass lists in properties and maybe different types.
On my side I didn't find any use of passing maps. The only component that does use a more complex structure for state is the tree but I opted for using a specialized tree library instead. The ones that use Vec will greatly benefit from IArray.
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.
Attributes
is more-or-less a map, although it's more specialized I guess. Overall, it makes sense to have such a thing in implicit-clone, though I confess I haven't looked at the implementation what you're currently using to implement it.
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.
Ah yes I forgot there is that Attributes thingy...
Well... yes probably Attributes is the same (at least same idea) than IMap but it shouldn't be a concern here. Attributes is used for internal machinery in Yew. Here we are talking about API and what tools we want to expose to the user. I don't know if anyone needs Attributes? Do they? Oh... it's public API... haha okay so maybe 😁 why is this public API?? xD (cc @siku2 maybe you know something? I think you've been maintaining Yew the longest)
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.
@hamza1311 I forgot to mention this but the map feature brings zero dependencies as it depends on indexmap which yew depends on anyway.
...come to think of it... 🤔 even if I didn't find a use in my use cases, it seems that having an immutable map collection is still something useful. IMap has 2 advantages:
- it implements ImplicitClone: this means all its API yields clones instead of references which is very useful for properties
- you can create static collections (for small collections)
I still removed the feature in the dependencies because it's optional anyway. I did it because I originally re-exported but now that we don't re-export there is absolutely no point. (tbh... I really don't get why we ban re-export 🤷♀️)
@WorldSEnder I just checked Attributes and it has quite some big differences:
- Attributes can only hold strings vs IMap can hold anything that implements ImplicitClone itself: it could be faster to use Attributes
- Attributes has a variant
Dynamic
which allow using subslices of an existing set of keys/values": it's probably used to optimize Yew somewhere... I'm not sure if IMap should also implement this, I don't see much value tbh.
Overall I have the feeling that Attributes is really specialized for Yew. The reason why it is public API is because it is used by yew-macro so it has to be.
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.
@hamza1311 oh nevermind what I said. We have to keep the feature map otherwise I can't make the impls for IntoPropValue which is a yew trait. Chicken and egg problem.
Unless there is a way to put a #[cfg()]
on the features of a child dependency, I don't see any other way. It's not a big deal anyway since it brings no dependencies.
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.
It doesn't matter either way. I'm fine with keeping it or removing it. I brought it up because I didn't see any uses of it
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.
Dropping this link to a comment here yewstack/implicit-clone#7 (comment)
It's interesting because @XX is making a widget library and he wants to allow the user to set/get attributes on a component.
cc @WorldSEnder
Co-authored-by: Muhammad Hamza <[email protected]>
This reverts commit 7c22326.
I'm not sure what I need to do to fix the tests 🤷♀️ |
# Conflicts: # packages/yew-macro/tests/html_macro/component-fail.stderr # packages/yew-macro/tests/html_macro/element-fail.stderr
I fixed them |
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.
Finally! We're ready to merge this in
Thanks a lot for the fixes @hamza1311 🤗 And thanks everybody for all your time on the matter! ❤️ |
This seemed to be a pretty substantial change to yew, would be awesome to read about it somewhere in the "next" docs :) |
Oh sorry I think the doc on that will be very succinct. It is worth a blog post in my opinion but every time I tried to explain the theory I feel like people did not get it. Maybe a post with an example that shows in which order the components are updated and when the memory is freed would be the best way to understand the concept. To summarize, it's two-folded:
|
@cecton do you want to write on up? |
@hamza1311 sure! xD There are so many things I say I will/would do. I should stop saying things I would do and start doing haha I will put that on my TODO list |
Description
Please check the example
immutable
in this PR to see how it is used.I have started some time ago an experiment of porting Immutable.js to Yew. This basically leverage the use of
ImplicitClone
on 3 core types:String
,Array
andMap
. And use garbage collection with Rc.&str
andString
indifferently and with Rc.Rc<IndexMap<K, V>>
or a&'static [(K, V)]
.Following the discussion in #2448 I realized that it is better to create a more generic "immutable" crate rather than crating a "yew-immutable" crate. This is why I created this new project: https://github.com/rustminded/imut It's currently not published on crates.io yet because I prefer getting the green light here first. If my PR here is approved I will add the documentation and publish
imut
. (I couldn't take the crateimmutable
because it is already taken and I couldn't reach the owner.)To avoid breaking compatibility I added a type alias from
AttrValue
toIString
. Those types were identical anyway.I also added a re-export of
imut
toyew::immutable
so people can import them easily if they want without the need to addimut
to their dependencies.It is best for the reviewers to also review https://github.com/rustminded/imut and put their remarks in this PR. The most important changes are there after all.
Fixes #2448
Checklist
cargo make pr-flow
imut
I have added a similar API of immutable.js to[edit: conflicting too much with rust's api... not a good idea, it would be confusing]imut
imut