engine icon indicating copy to clipboard operation
engine copied to clipboard

feat: Support chain for `attributes.add` and `registerScript`

Open eXponenta opened this issue 4 years ago • 1 comments

This feature allowing use chaining in script registration phase.

What is problem:

Current implementation of ScriptAttributes.add and pc.registerScript return void and this enforce use explict syntaxis:

class Class  extends pc.ScriptType {

}

pc.registerScript('Class', Class);

Class.attributes.add(...);
Class.attributes.add(...);
Class.attributes.add(...);

What is new:

Because add and registerScript will return this and script, we can use it for chaining.

  1. We can use anonymous class as argument: const Class = pc.registerScript('Class', class extends pc.ScriptType {} )
  2. We can chaining attributes: Class.attributes.add('attr1', ...).add('attr2, ....)
  3. We can doing both it together:
    pc
        .registerScript('Class', class extends pc.ScriptType {} )
        .attributes
        .add(...)
        .add(...)

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

eXponenta avatar Jul 11 '21 11:07 eXponenta

We've had a chat about this in another place, and I personally believe this is a bad API in this particular case. Reasoning:

  1. Chaining should serve functional meaning, like chaining tween animations, routing middleware, or vector maths due to simple methods and often need a lot of it in a row.
  2. Chaining leads to returning this which then blocks API design to such decision, and changing it will not be possible.
  3. Multi-line chaining - is difficult to read and leads to unconventional indentation, especially when there are objects or functions defined in each call (look at events).
  4. Multiple ways of doing the same thing - lead to a steeper learning curve.

An example where chaining which was a good idea at first glance, lead to regret later: events chaining. Defining events in a chain makes no functional purpose, as it is exactly the same as defining events one after the other, also prior is cleaner to read. But it leads to that on returns this, while there is a useful pattern to return event handler, which could be used to unsubscribe from events. But we cannot do that anymore.

So if chaining - does not implement a functional meaning, and the same functionality is already possible, then adding an alternative coding style which will introduce unnecessary code differences - personally, I would not implement it.

Maksims avatar Jul 12 '21 07:07 Maksims

Hi @eXponenta - thanks for the PR, but after the discussion above, I agree this does not provide advantage over what the API currently does. I'm closing this.

mvaligursky avatar Mar 17 '23 08:03 mvaligursky