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

Hydration overrides hydrated values when defaults are state objects. #243

Closed
krypt102 opened this issue Mar 2, 2023 · 6 comments
Closed
Labels
broken Something isn't right not ready - blocked Waiting on other work to be done

Comments

@krypt102
Copy link
Contributor

krypt102 commented Mar 2, 2023

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 krypt102 added broken Something isn't right not ready - evaluating Currently gauging feedback labels Mar 2, 2023
@krypt102
Copy link
Contributor Author

krypt102 commented Mar 2, 2023

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

@Dionysusnu
Copy link
Contributor

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.

@Aloroid
Copy link
Contributor

Aloroid commented Mar 9, 2023

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

@krypt102
Copy link
Contributor Author

krypt102 commented Mar 9, 2023

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.

@dphfox
Copy link
Owner

dphfox commented Mar 10, 2023

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 dphfox added not ready - blocked Waiting on other work to be done and removed not ready - evaluating Currently gauging feedback labels Mar 10, 2023
@dphfox
Copy link
Owner

dphfox commented Apr 15, 2024

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

@dphfox dphfox closed this as completed Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broken Something isn't right not ready - blocked Waiting on other work to be done
Projects
None yet
Development

No branches or pull requests

4 participants