Fusion icon indicating copy to clipboard operation
Fusion copied to clipboard

Hydration overrides hydrated values when defaults are state objects.

Open krypt102 opened this issue 2 years ago • 5 comments

This is most likely similar to #171, however it's a bit hard to explain so I'll post a code example below. In one of my teams games, we build our UI based off of Themes, these themes have properties such as font, text, background, etc.

This is an example of how a TextLabel component works:

function Components.TextLabel(data)
	return Hydrate(New "TextLabel" {
		AutomaticSize = Enum.AutomaticSize.XY,
		FontFace = Theme.font,
		TextColor3 = Theme.text,
		BackgroundTransparency = 1,
		TextSize = Theme.fontSize,
	})(data)
end

For our spectate system, we use this textlabel component but override the TextColor3 to be a blank white.

Components.TextLabel {
    Text = "No players available to spectate.",
    TextColor3 = Color3.new(1, 1, 1),
}

We build it this way so that we can have a base component and re-hydrate any custom properties we need. This poses a very subtle error though, notice how in the Textlabel component function we set TextColor3 to Theme.text, when the Theme changes (which happens on load and when the user manually changes), it re-overrides hydrate to be the Text color, not the white color that is expected.

Here's a step by step method of what exactly happens:

  1. UI is built and textlabel is set to white
  2. Theme loads and the font value is changed
  3. Hydrate doesn't clean up the old observer bind internally, and changes the textlabel to be the wrong colour.

I believe the main cause of this problem is found here.

krypt102 avatar Mar 02 '23 07:03 krypt102

I believe this issue may also apply to attributes, where binds aren't deleted either.

krypt102 avatar Mar 02 '23 07:03 krypt102

I don't think this is so much a bug as a bit of unexpected design: Property binds are not unique across Fusion, ie multiple states or raw values can be assigned to the same instance property, but older binds for that same property still remain active.

Dionysusnu avatar Mar 03 '23 15:03 Dionysusnu

Hydrate isn't intended to be used for this use-case where you Hydrate a element already managed by Fusion.

Aloroid avatar Mar 09 '23 06:03 Aloroid

Hydrate isn't intended to be used for this use-case where you Hydrate a element already managed by Fusion.

My purpose for using it was to add custom properties to base objects, as there's no pre-made system in fusion this seemed like the only solution without needing to define every property.

krypt102 avatar Mar 09 '23 07:03 krypt102

I talk about this use case explicitly in the context of #206, which nullifies this issue if it goes through:

There do exist other use cases of Hydrate for such things as allowing users to define arbitrary properties on components, but this is generally not considered to be a good idea as it can leak the internal details of how the component works by turning hidden instance properties into part of the component's own public API. Most often these solutions are intended to reduce the redundant boilerplate of assigning common properties such as Name or Parent, but these use cases would ultimately be better served by dedicated APIs for integrating common sets of properties automatically. Since this is merely boilerplate reduction for convenience, and not what I would consider to be a theoretically sound use case for Hydrate, I'm choosing not to include this in scope of this change. However, we should probably still address this.

As such, this issue is currently blocked and not likely to actually ever see the light of day given the other issue is likely to go through.

dphfox avatar Mar 10 '23 05:03 dphfox

Hydrate is being removed, so this double assignment will no longer be an issue. Closing this issue.

dphfox avatar Apr 15 '24 13:04 dphfox