Fusion icon indicating copy to clipboard operation
Fusion copied to clipboard

Allow State to Be Used for "OnEvent" and "OnChanged"

Open The-Big-Bird opened this issue 4 years ago • 12 comments

Would be great if we could use state for "OnEvent" and "OnChanged" props to get rid of unnecessary connections when they aren't needed.



local function button(props)
  local function onPress()
    print("Pressed!")
  end  
  
  return New "TextButton" {
    [OnEvent "MouseButton1Down"] = Computed(function()
      return props.Pressable:get() and onPress or nil
    end);
  }
end


The-Big-Bird avatar Dec 28 '21 01:12 The-Big-Bird

What's the advantage of doing this over something like the following?

local function button(props)
  local function onPress()
    print("Pressed!")
  end  
  
  return New "TextButton" {
    [OnEvent "MouseButton1Down"] = function()
        if props.Pressable:get() then
            onPress()
        end
    end
  }
end

I would consider this code easier to read.

dphfox avatar Dec 28 '21 01:12 dphfox

The ability to not occupy memory for the connection needlessly.

In this example that difference would be minuscule, but I'm currently writing a button component with 4 different input connections and some of the instances of the button call for it to be unclickable.

The-Big-Bird avatar Dec 28 '21 01:12 The-Big-Bird

Is this a significant memory overhead? This smells like premature micro-optimisation.

I much prefer implementing features that have tangible developer experience benefits over those that address unproven performance issues, which is why I ask :)

dphfox avatar Dec 28 '21 01:12 dphfox

This feels like the right issue. What would be nice is being able to use OnChanged to fire an event every time that value is changed. In general this would allow for cleaner code and a more dynamic way of looking at states.

For example, if you had a page changer, every time the value changes it checks if it's possible to go back another page or forward another page and disable/hide the buttons if they cannot be used.

Rather than the reactive approach of having to check both before running the change and after the change to check for next time.

railworks2 avatar Jan 24 '22 15:01 railworks2

In general this would allow for cleaner code and a more dynamic way of looking at states.

For example, if you had a page changer, every time the value changes it checks if it's possible to go back another page or forward another page and disable/hide the buttons if they cannot be used.

This already exists in the form of the Observer class.

Dionysusnu avatar Jan 24 '22 20:01 Dionysusnu

Thank you. It's a shame this is not part of the documentation

railworks2 avatar Jan 24 '22 21:01 railworks2

Thank you. It's a shame this is not part of the documentation

@railworks2 it used to be Compat but renamed to Observer

you can find the api reference here

Barocena avatar Jan 29 '22 00:01 Barocena

I think this is something worth supporting in the future :)

dphfox avatar Jan 29 '22 16:01 dphfox

Thank you. It's a shame this is not part of the documentation

As far as I can tell, the documentation was written all at once in one pass, and is enough to get you started. However it's not going to be actively maintained until Fusion starts getting closer to an initial release. Luckily @Elttob made it REALLY good for Luau intellisense, so it's really easy to use the library just like you would in any traditional IDE.

LoganDark avatar Feb 06 '22 23:02 LoganDark

Thank you. It's a shame this is not part of the documentation

As far as I can tell, the documentation was written all at once in one pass, and is enough to get you started. However it's not going to be actively maintained until Fusion starts getting closer to an initial release. Luckily @Elttob made it REALLY good for Luau intellisense, so it's really easy to use the library just like you would in any traditional IDE.

It's more that the wiki refers to it as for dealing with non Fusion code, I got confused. The recommendation given related to something that I didn't know had a name change and so when I said that I did not know the old name.

I'm sure it's on the list of things to work on, just me misunderstanding something and not constantly reviewing all changes.

railworks2 avatar Feb 07 '22 00:02 railworks2

Yeah - the Fusion docs were largely one big push back in August to get some semblance of content there. They need to be updated so I'll spend some time before v0.2 writing up more stuff :)

dphfox avatar Feb 07 '22 06:02 dphfox

To elaborate on my thoughts around this idea:

I think that you can absolutely abuse this to write code that's less easy to read, or chase needless performance gains that don't matter in the grand scheme of things. However, I've never truly been concerned about the ability for any of my APIs to support this kind of micromanagement, because the reality is: if someone wants to fruitlessly micromanage their code, they can probably do it no matter what API you throw at them. Micromanagement is something to be solved at the project management level fundamentally - as API designers, we should be making things as simple as possible, but making everything childsafe only goes so far. You have to draw the line somewhere, otherwise you tread on the toes of people that know what they're doing and have good reason to do it.

Instead, what I'm most interested in is the 'path of least resistance', and putting some rails on that to try and push most people towards a good idiomatic path. What is the least effort to write or think about? That's what most people are going to implement. I design all of my APIs around this concept right now.

The path of least resistance for creating an event listener that acts conditionally, is to create an event listener with a conditional. It's almost self-evident, so I have full confidence that most people will continue to use event listeners with conditionals inside them. The overhead of an extra event dispatch and processing an if-condition probably isn't even significant for most people.

However, most people is not all people, and 95% is far away from 100%. There are absolutely cases in Roblox where you need to be careful around event execution, primarily when you're dealing with large quantities of instances reacting to events. In these cases, it may be desirable to swap out or disable handlers ahead of time, so that you can cut down on event dispatches. I think this is worth supporting.

We should keep in mind here that state objects are also more open ended than simpler Computed statements that switch on a boolean to return a callback or nil. Enabling state object integration here allows for callbacks to be piped in from anywhere. This may make integration with existing state object pipelines easier by eliminating some boilerplate code at the end of the pipeline that only exists for the purpose of forwarding on calls. This may be useful when building reusable component libraries or other kinds of third party Fusion libraries where you're often dealing with data flowing in from external state objects.

You might notice that I'm using lots of 'may' and 'can' here. The reason why is because this is experimental stuff, right? Most of the stuff I'm discussing here is my own theory and conjecturing, so to see how much of this holds up, we really need to try it out. If it becomes a problem for code readability or confuses new users, we can redesign it or remove it.

But mostly I care about opening the doors wider for power users here. I don't think it should harm newer developers, and Fusion is already easily micromanageable in many ways, so I don't think it'll do much there either.

dphfox avatar Feb 12 '23 09:02 dphfox