search

leopard-js / leopard

Library for making Scratch-like projects with JavaScript.
https://leopardjs.com
MIT License
136 stars 26 forks source link

Add new, cleaner Trigger syntax #177

Closed towerofnix closed 1 month ago

towerofnix commented 1 year ago

Development

Description

Example new output:

this.triggers = [
  Trigger.greenFlag(this.whenGreenFlagClicked),
  Trigger.clicked(this.whenthisspriteclicked),
  Trigger.startedAsClone(this.startAsClone),
  Trigger.keyPressed({ key: "space" }, this.whenKeySpacePressed),
  Trigger.keyPressed({ key: "any" }, this.whenKeyAnyPressed),
  Trigger.backdropChanged(
    { backdrop: "Beach Rio" },
    this.whenbackdropswitchesto
  ),
  Trigger.backdropChanged(
    { backdrop: "Field At MIT" },
    this.whenbackdropswitchesto2
  ),
  Trigger.loudnessGreaterThan({ VALUE: 10 }, this.whengreaterthan),
  Trigger.timerGreaterThan({ VALUE: 3 }, this.whengreaterthan2),
  Trigger.broadcastReceived({ name: "message1" }, this.whenIReceiveMessage1)
];

Testing

Tested manually with this project: https://scratch.mit.edu/projects/817687184/

towerofnix commented 1 year ago

Self-assigning for symbol deprecation notice, I'm going to only give notice when the triggers are created (i.e. sprite/stage is constructed) so we don't constantly spam the console during project interpretation. That's detectable too (trigger constructor already differentiates symbol/function) and should cover everything except custom uses of (a trigger).match against symbol — it's quite unlikely anyone was using that method in practice & we will document its changes in the 2.0.0 release.

towerofnix commented 1 year ago

I've added a deprecation notice which displays once for each deprecated symbol:

Trigger.GREEN_FLAG syntax is deprecated - it'll be removed in a future version of Leopard. Use the new syntax...
adroitwhiz commented 1 year ago

I think there might be a way to better integrate this with TypeScript--I'll have a go at a prototype implementation soon

adroitwhiz commented 1 year ago

I've been putting this off for a while because the new syntax still doesn't feel right to me, and I think I'm slowly working towards why?

Fundamentally, triggers are like the JS concept of event listeners (putting aside edge-activated triggers for a moment)--when a certain event occurs, like the user clicking the green flag or pressing a certain key, the sprite wants to call its own function in response to that.

In JS-land, there are two main ways to add an event listener. The first is using addEventListener with the event's name:

button.addEventListener('click', this.handleButtonClicked);
player.addEventListener('keypress', this.handleKeyPressed);

This lets you add as many event listeners to one target as you want. Notice how the event name is passed in as the first argument--this is somewhat similar to the current Trigger syntax.

The second way is using event handler properties, which only let you attach one listener per event type, as setting the property will overwrite the previous listener:

button.onclick = this.handleButtonClicked;
player.onkeypress = this.handleKeyPressed;
// This will remove handleButtonClicked
button.onclick = somethingElse.doSomethingElse;

Notice how both these examples have the same word order: target, event, handler. This is probably something that Leopard should should attempt to do as well. Right now (before this PR), triggers look like this:

this.triggers = [new Trigger(Trigger.CLICKED, this.whenstageclicked)];

This has the event and the handler, but not the target. In this case, the target is the project. If we translated this into DOM API language, using the same "button click" example from earlier, this would look something like:

this.eventListeners = [new EventListener('click', this.onButtonClicked)]

This is similar to the addEventListener example above (event name followed by listener), but a bit more awkward--which button are we attaching the listener to?

My concern is that this PR's syntax would align even less with event listener syntax. Here's what it would look like with the "button clicked" example:

this.eventListeners = [EventListener.click(this.onButtonClicked)]

This seems a bit backwards when you look at it from the perspective of event listeners. You'd need to hold all event types as static properties on a single class, which discards information about which types of objects can fire which events. This might be fine for Leopard since all events are fired from the project, but would break the principle of encapsulation for most other APIs--one class's "click" event might mean something very different from another class's "click" event, and the two could have very different signatures.

Ideally, the existing triggers would be made into event listeners on Project, but that would require the Project to be passed into sprites' constructors, which would require a rework of the project-loading API.

I don't really have a good answer as to what the best syntax for triggers is, but I do worry that this isn't it. I'd be interested in hearing others' thoughts.

PullJosh commented 1 year ago

Brainstorming here... How would you feel about some kind of "addEventListener" syntax? It would be a little magical because it would accept generator functions as input (rather than a plain callback), but otherwise could be pretty similar to the DOM apis.

class MySprite extends Sprite {
  constructor() {
    // ...
    this.addEventListener("click", this.whenThisSpriteClicked);
    this.stage.addEventListener("costume", this.whenBackdropSwitches, { name: "backdrop2" });
    const listener = this.addEventListener("broadcast", this.whenIReceive, { message: "message1" });

    // Maybe also allow something like this...
    this.removeEventListener("broadcast", this.whenIReceive, { message: "message1" });

    // ...or this...
    this.removeEventListener(listener);
  }
}

This would also allow attaching and removing listeners while the project is running in a more JS-y way.

towerofnix commented 1 year ago

Thanks for your remarks, both!

@adroitwhiz, I agree. Triggers are at their core an acceptably JavaScript-y concept — defining handler functions which get called later, potentially multiple times, according to certain events/conditions — but the way we use them in Leopard is currently at best an approximation of standard JS form, and this PR goes further away still. I agree that's something to address!

You'd need to hold all event types as static properties on a single class, which discards information about which types of objects can fire which events. This might be fine for Leopard since all events are fired from the project, but would break the principle of encapsulation for most other APIs--one class's "click" event might mean something very different from another class's "click" event, and the two could have very different signatures.

Something to note is that JavaScript is... kind of awkward here? Because event listeners are a long-standing system which predate the more extensible patterns found in classes and web standards introduced post-ES6, they're incapsulated insofar as each class (usually HTMLElement) has its own event names and signatures and subclasses may define their own (ex. HTMLFormElement's "submit"). But naming and signature conflicts are possible (with no system of management like ES6 super offers), and there's a variety of utilities which are completely unheard of outside of events (terms like bubbling, propagation, options provided directly to addEventListener like capture, passive...). It's an involved system which, like, theoretically makes sense and is very powerful, but is also just not friendly to learning programmers.

In other words, there's a benefit to modelling based on JavaScript's system (in that we teach new, but important, and soon familiar syntax), but we also should be conscious of how we implement it so that we're teaching and exposing useful capabilities to advanced users while not bogging new users down in minute semantics. I don't think anyone was proposing to just pull JS's event listener system identically anyway — just putting the note out there of why we might not want to!

Ideally, the existing triggers would be made into event listeners on Project, but that would require the Project to be passed into sprites' constructors, which would require a rework of the project-loading API.

I'd like to think more about Leopard events individually, but in general yes, I think there's a benefit to exposing Project (as an event target) to sprites. The alternative is exposing "public" events via the stage (ex. the stage re-emitting any broadcast events emitted by other sprites), but I'd generally like to avoid arbitrary differentiation between stage and sprite, especially on core behavior like how events are handled.

Brainstorming here... How would you feel about some kind of "addEventListener" syntax? It would be a little magical because it would accept generator functions as input (rather than a plain callback), but otherwise could be pretty similar to the DOM apis.

@PullJosh Like I mentioned above, I think addEventListener is a syntax worth using — it's certainly more commonplace than the other standard JS way, onclick / onbroadcast etc, which allow only a single handler and so wouldn't work for Scratch events anyway. Apart from custom interfaces for web apps (React, etc), the addEventListener syntax is practically universal (even Node uses the same basic idea, just with a more compact syntax, idiomatically .on/.removeListener). So following the same basic model exposes users to patterns that will become familiar, unlike the existing relatively unique forms currently in Leopard and as implemented in this PR.

/* A */ this.addEventListener("click", this.whenThisSpriteClicked);
/* B */ this.stage.addEventListener("costume", this.whenBackdropSwitches, { name: "backdrop2" });
/* C */ const listener = this.addEventListener("broadcast", this.whenIReceive, { message: "message1" });

/* D */ this.removeEventListener("broadcast", this.whenIReceive, { message: "message1" });
/* E */ this.removeEventListener(listener);

...Fair warning: I'm going to use your example as a starting point for discussing the syntax specifics, which means digging into this code a lot 📦

While I mentioned that JavaScript has a lot of nuanced complexities for event handling that we may not want to reflect fully here, I do think that if we're going to implement a version of addEventListener at all, we need to get the essentials right — which means matching JavaScript patterns as closely as reasonably possible (and applicable).

In JavaScript, addEventListener does not return anything. It doesn't return a "listener" identifier (e.g. the same way setTimeout or setInterval return identifiers that can later be passed to clearTimeout and clearInterval). To remove a listener, you refer to it by the specific handler function object (inherently unique) that was provided to addEventListener.

this.addEventListener("click", this.whenThisSpriteClicked);
this.removeEventListener("click", this.whenThisSpriteClicked);

There is no other syntax for removing event listeners. There are also only two (related) ways to add an event listener: name then function, or name then function then options. This is basically reflected in your lines B and C above... but not really, because the scope for the options argument is completely different. In JavaScript, the options argument covers capture, once, passive, and (abort) signal. These are all options which apply to every event listener, and control the basic behavior of the listener (see MDN). They distinctly do not provide a "filter" for events like your example proposes.

I believe that's an arbitrary code pattern which we would do more inconvenience than good to teach new JavaScript learners. It's just not reflective of the standard. This goes for the listener = addEventListener(...) / removeListener(listener) syntax as well.

Of course, the trouble is that it's nice, and the alternative just isn't going to be as nice. For example, consider the "costume" event for "when costume switches to..." / "when backdrop switches to...", your line B. In JavaScript the way to implement it is with an early return (or other conditional clause) in the handler function:

constructor() {
    ...
    // If something else about this code seems wrong, I'll get to it shortly 👻
    this.stage.addEventListener("costume", this.whenBackdropSwitches);
}

whenBackdropSwitches() {
    if (this.stage.costume !== "backdrop2") {
        return;
    }

    // do something
}

I think there are node libraries which implement their own custom filtering behavior, but it's non-standard. The only filter option provided in standard JavaScript is the event name itself. All other filtering must be done by the listener function.

Another awkward note, particularly when adding a listener to another object, is the definition of this. See MDN for details. Basically, under normal circumstances, this on a handler function is automatically bound to the target of the event (in our case the stage, the project, or another sprite). There isn't any getting around this as JavaScript doesn't bind functions to a this value at all if referred to "anonymously" (without a dot accessor like this.doSomething() or this.stage.doSomething()):

class Foo {
    getThis() {
        return {myself: this};
    }
    getGetThis() {
        return this.getThis;
    }
}

foo = new Foo();
foo.getThis(); // myself === foo
foo.getGetThis()(); // myself === undefined

It's technically fine for events assigned to the self (e.g. this.addEventListener("click", this.doSomething)), but in all other cases we need to use a helper function or specific binding:

this.stage.addEventListener("click", () => this.whenStageClicked);
this.stage.addEventListener("click", this.whenStageClicked.bind(this));

And now you run into the hilarious issue of function identity for removal, because... obj.method.bind(obj) !== obj.method.bind(obj)! 🥳 

class Foo {
    excitability = 10;
    handlePartyInvitation() { console.log("Yay" + "!".repeat(this.excitability)); }
}

foo = new Foo();
bar = new EventTarget();

bar.addEventListener("invite", foo.handlePartyInvitation.bind(foo));
bar.dispatchEvent(new Event("invite")); // Yay!!!!!!!!!!

bar.removeEventListener("invite", foo.handlePartyInvitation.bind(foo));
bar.dispatchEvent(new Event("invite")); // Yay!!!!!!!!!!

This obviously goes for arrow functions as well, where (() => this.method()) !== (() => this.method()).

I don't know if there is a standard way to handle this. It's absolutely one of the messinesses of JS's event handling system (and/or its this-binding system in general).

I generally do this:

class Foo {
    constructor() {
        this.bindEventHandlers();
        this.addEventListeners();
    }

    teardown() {
        // Called e.g. when a clone is deleted
        this.removeEventListeners();
    }

    bindEventHandlers() {
        // Get ready for boilerplate...
        this.handlePartyInvitation = this.handlePartyInvitation.bind(this);
        this.handleStageClicked = this.handleStageClicked.bind(this); // etc
    }

    addEventListeners() {
        // This function MUST be called AFTER bindEventHandlers.
        something.addEventListener('invite', this.handlePartyInvitation);
        stage.addEventListener('click', this.handleStageClicked);
    }

    removeEventListeners() {
        // This references the same bound functions defined before being added as listeners,
        // so the functions are identical and are removed correctly here.
        something.removeEventListener('invite', this.handlePartyInvitation);
        stage.removeEventListener('click', this.handleStageClicked);
    }
}

The current system cheats by understanding who this should be since it is the same object whose triggers is currently being iterated. This means it's impossible for an object to add one of its own handler functions as a lister on another object. I strongly feel this should be addressed and made room for, as IMO the greatest advantage of JavaScript over Scratch (within a sprite-based framework... like Scratch!) is treating sprites as data, directly analogous to OOP constructs in the wider world of JavaScript and programming in general. In other words, it's currently impossible for a clone to refer to events on its creator, a creator to events on its clone, a sprite to events on the stage, the stage to events on specific sprites, etc; these are all practical situations which I believe we should support.

Something to note is that some form of a teardown function would be strictly required because if you can add event handlers to other objects, they're no longer encapsulated on the one object, so when that object gets disabled (i.e. a clone is deleted), there is no way to trace listeners it created at any point in its lifetime back to the object (and then remove those listeners). A general-purpose workaround would be non-standard (such as a custom addEventListener form).

Unfortunately, all this adds a fair amount of overhead via boilerplate to JavaScript code — in all but the most simple use cases (self-listeners avoid the issue... but sprites adding listening for events on the stage or project already brings it up, you don't even need complex behavior like dynamically adding and removing listeners). I don't believe it's possible to avoid the majority of it due to the way JavaScript fundamentally behaves (this kind of boilerplate necessarily plagues any library built on native EventTargets), but am interested in alternate perspectives on this fact of the matter.

Is this boilerplate OK? Since this is just how JavaScript is, do we really want to shroud it or look far away? Or is it all too much for new users working with their generated Leopard code, introducing an unfortunately involved system of "event handlers" too quickly and bringing in excruciatingly awkward nuances like this binding in a context where they just shouldn't matter, even though they do?

PullJosh commented 1 year ago

@towerofnix Thank you for all the thoughts.

There is definitely room for disagreement here, but I am leaning towards feeling like we would be better off trying to hide the ugly this/binding behavior from users where possible so that they can focus their attention on more basic concepts.

To that end... We could automatically bind all of a target's methods automatically as soon as it is created. (Essentially performing nix's bindEventHandlers() automatically.)

The following code makes this auto-binding completely transparent to the user:

// Within the Leopard source code
class Target {
  constructor() {
    // ...
    this.autoBindAllMethods();
  }

  getAllMethods() {
    return Object.getOwnPropertyNames(this)
      .filter(property => typeof this[property] === "function");
  }

  autoBindAllMethods() {
    const methods = this.constructor.prototype.getAllMethods();
    for (const name of methods) {
      this[name] = this[name].bind(this);
    }
  }
}

// Now this works transparently for Leopard users
class MySprite extends Sprite {
  constructor() {
    super();
    this.stage.addEventListener("click", this.whenStageClicked);
  }

  whenStageClicked() {
    console.log("I am", this); // this === instance of MySprite
  }
}

I have a complete, working proof of concept here.

I am not completely convinced that this is the right move, but I'm also not necessarily convinced that it isn't. 😜

With the auto-binding done, I'm pretty sure the following works as expected in all cases, right?

this.stage.addEventListener("click", this.whenStageClicked);
this.stage.removeEventListener("click", this.whenStageClicked);
towerofnix commented 1 year ago

@PullJosh Yes, auto-binding gets most of the way there! Thanks for the discussion.

Another hurdle to consider is removing events from other objects when a clone is deleted. We can implement "magic" behavior to handle this, in a couple of different ways:

  1. In autoBindAllMethods, assign a property on the function to this; when deleting the clone, iterate over all live targets (sprites/clones/stage) and automatically remove their event listeners whose function's property matches the clone being removed.
    • This is a fairly straightforward method but I'm not convinced it'd be that performant, particularly for projects creating and deleting lots of clones every frame.
  2. In autoBindAllMethods, assign a property on the function to an empty array; in addEventListener (on a target), add this (the target) and the event name as a pair in the handler function's array property; when deleting the clone, iterate over each function's array property and call removeEventListener on the pair's target with the pair's event name and the function.
    • Maintaining the state of that array is important, so obviously the target-event pair needs to be removed from the function when removeEventListener is called.
    • We need to ensure the target-event pairs don't ever reference a target which has been deleted, since that would be a memory leak (and incorrect state). What the steps described above don't catch is some target adding an event listener to a clone, and then that clone being removed; the original target's function's target-event pairs would still include a reference to the clone as the clone itself wasn't tracking where it was referenced.
      • This should be trivial to fix, though, just with an extra step when deleting a clone: iterate over all event listeners on the clone and call removeEventListener. As mentioned above, this would remove the relevant target-event pair from the function's array, scrubbing references to the clone.
    • I don't actually know that an array is the best data structure for this, I'm not versed in comparing performance demands of similar operations in different data types!

@adroitwhiz This definitely isn't something we've made a final decision on! But it seems like if we go with addEventListener-type syntax, there is a lot of "magic" we need to do to avoid introducing the user to tons of intricacies and abstract behavior which, although arguably relevant in the real world, would almost certainly just overwhelm learning users. See implementation discussion from @PullJosh and me.

If this is a syntax direction you're interested in exploring, I'd be interested to hear your thoughts on the implementation or specifics of that "magic" — you're generally more experienced with data structures and the ways objects get tied to each other than me so I'd like to hear your thoughts, so we can have code that is both practically performant and not so complex as to befuddle us or anyone working on it in the future lol.