Optimize the default change implementation

[kellytk]

Return false if Self::Properties has a value of ().

[kellytk]

Cloning the repo and opening it in VS Code generated several errors. I was therefore given the mistaken impression when this code didn't generate additional errors that it was correct. There are several solutions however none are as straight forward as I would like so I'll await comment from @jstarry.

[hgzimmerman]

I'm not entirely convinced that this can be accomplished without making a breaking change, or adding additional baggage to the Properties trait in the form of a default method.

I think the choices for implementing something like this are either:

TypeId::of::<Self::Properties>() != TypeId::of::<()>()

or

pub trait Properties {
    ...
    fn default_should_render() -> ShouldRender { true }
}
impl Properties for () {
    ...
    fn default_should_render() -> ShouldRender { false }
}

The first of which requires another bound on Component::Properties, and the latter (obviously) adds another method to Properties - which I don't think is particularly bad, although I think it adds clutter.

I think the benefit is relatively small - only a slightly more sane default for a method most people should be overriding anyway.

[kellytk]

a method most people should be overriding anyway

Why should it be overriden when type Properties = ();? In one of my apps with 29 components only 12 are passed properties. Adding the same boilerplate to 17 components, more than half, is tedious and seems wasteful.

[hgzimmerman]

I was probably speaking too broadly - Its dependent on the style of application used - split between the monolithic components passing relevant state downwards and agent-based state distributed as needed through bridges. The latter requiring fewer custom props, and the former requiring props in most cases.

When you start writing component libraries by wrapping elements with default CSS provided by a CSS framework - literally everything requires custom props.

---

I think the spirit of reducing number of lines needed to be written is good, especially when it can help avoid a hidden performance foot-gun like this - but its my personal opinion that it isn't worth making breaking changes for - at least at the moment.

[kellytk]

Why is it your opinion that the twin benefits of reducing boilerplate and improving the efficiency of default behavior are insufficient to introduce a breaking change?

[hgzimmerman]

Because it is somewhat rare, the performance benefit is minimal, and if the performance problem is significant enough, its easy enough to sprinkle the one line:

fn change(&mut self, _props: Self::Properties) -> ShouldRender { false }

throughout a codebase until the problem is resolved.

---

I'm not opposed to something like this making its way in, and it ultimately isn't my say - I personally would be in favor of the second suggestion I listed above, although the first one is cleaner and more idiomatic - it would result in a breaking change, which I don't think is justifiable. Adding an extra trait bound to an associated type just so it can have slightly better default behavior may pose a burden to the core use case of the API. It represents a trade-off in a loosely defined space that I'm not able to weigh the costs and benefits - hence the conservative stance I have.

I do suggest giving that approach a shot in the meantime though.

---

I'm being proactive in bringing up potential justifications for not merging this, when that isn't really necessary. I feel that my preemptive pontification on the relative value of PRs like this comes across as dismissive, and I'll try to tone that down in the future.

[kellytk]

I'm being proactive in bringing up potential justifications for not merging this, when that isn't really necessary

You're a regular contributor to the project so irrespective of whether or not it's necessary, I appreciate it even if I personally disagree as I do in this case.

[jstarry]

The first of which requires another bound on Component::Properties

@hgzimmerman I'm confused by this statement. It looks like TypeId is a great approach to me

[hgzimmerman]

I hadn't personally tried the suggested change myself, thinking that the 'static bound required for the TypeId approach would require modification of either Component::Properties associated type or the Properties trait to accommodate it. I'm glad to see that it doesn't. I probably should have verified that first before using that as the basis of my opinion on the matter. 😞

[jstarry]

Haha no worries! Yeah, I was thinking the same thing. I don't really understand why that trait bound isn't needed explicitly but happy it isn't 😉

[jstarry]

Thanks for getting this started @kellytk!