-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
Treating constants like state objects #35
Comments
This is one of the first things that I made a utility function for when starting with Fusion, and would be a great addition. Being part of the public namespace / main module exports is something I wouldn't be a fan of, however. I like how rocket.rs does it, splitting main functionality and utility into Another useful utility function that I've been using with Fusion, which functions similar to the proposed local function unwrapState(stateOrValue, registerAsDependency)
if stateOrValue is a state then
return stateOrValue:get(registerAsDependency)
end
return stateOrValue
end |
Edit: After diving more into the Fusion source code, I now understand what you mean when you say that you want the ability to Statify constants. To that end, I would argue that while this might improve the programming experience a little bit (at first the separation of various things into states and constants was a bit confusing and led to a few bugs in my early contribution process), I would argue that it's better to separate them for optimization sake. Though, that's just my immediate thought. For instance, take the local disconnect = Observer(value):onChange(function()
if ref.instance == nil then
if ENABLE_EXPERIMENTAL_GC_MODE then
if conn.Connected then
warn("ref is nil and instance is around!!!")
else
print("ref is nil, but instance was destroyed")
end
end
return
end
Scheduler.enqueueProperty(ref.instance, key, value:get(false))
end)
table.insert(cleanupTasks, disconnect) This is completely unnecessary for constants, and would just result in us creating a lot of unnecessary objects. (Think creating an Observer object for every single property in the entirety of someone's UI code). For that reason I don't think this is viable long-term. If anyone thinks otherwise feel free to let me know! |
Do you have benchmarks to show that a function like this poses any performance issue? Optimisation should not a primary concern until it's proven to be a problem with data. In my mind it's much more important that developers have a good experience with Fusion than it is to always run the most optimal code path theoretically possible, though of course this is not an excuse to write horribly slow code.
This would not be used in such a fashion. For one, this is a user-facing utility, not something for use inside Fusion itself. Secondly, it's not mandated to be used, so where performance is needed you can manually handle both cases. Thirdly, most uses for statify would be when components are processing custom props - which is largely limited to one-offs - and not used for every state object. Fourthly, the overhead is entirely at create time - constant-value state objects are completely weightless in terms of run time performance due to the push nature of the reactive graph. |
The optimization basis was specifically in reference to memory usage, rather than speed. That being said, I misunderstood what you meant by this, and thought you were specifically referring to what we were doing internally. As a result all I can say is disregard what I said... totally my fault. |
I've so far identified three different solutions to this problem, which may not necessarily be the same as the solution proposed here.
Some pseudocode examples: local const = "const"
local state = Value("state")
-- option 1
local function makeState(x)
return if x is state then x else { get = function() return x end }
end
local function process(x)
print(x:get())
end
process(state) -- prints "state"
process(makeState(const)) -- prints "const"
-- option 2
local function get(x)
return if x is state then x:get() else x
end
local function process(x)
print(get(x))
end
process(state) -- prints "state"
process(const) -- prints "const"
-- option 3
local function wrap(component)
return function(props)
local realProps = {}
for key, value in pairs(props) do
realProps[key] = makeState(value)
end
return component(realProps)
end
end
local component = wrap(function(props)
print(props.value:get())
end)
-- prints "state"
component {
value = state
}
-- prints "const"
component {
value = const
} I'm personally a huge fan of how option 2 doesn't require transforming variables or constructing objects, but it also means we can't use OOP notation. Perhaps we should be reconsidering the OOP syntax of state objects entirely? We're already moving away from metatable-based OOP, which would make |
So I came up with this stupid idea. (But I'm still going to say it 🙂) |
This has been formalised as the |
As mentioned in this Twitter thread (https://twitter.com/Elttob_/status/1432242081368530949):
Hypothetical usage:
The text was updated successfully, but these errors were encountered: