Remove behavior names, support multiple parameterized behaviors

[balderdash]

I'm removing the name property from behaviors now that method comparisons are working 🎉

However, I'm re-complicating things a bit to properly support parameterized behaviors such as follow <target>.

Adding and removing a function defined in the global scope from a behaviors list is pretty straightforward, because functions equal themselves. However if you want to to have a behavior that varies based on a argument, we run into a problem:

function follow(target) {
  return function(sprite) {
    sprite.moveToward(target);
  }
}
sprite.behaviors.push(follow(cat));
sprite.behaviors.indexOf(follow(cat)); // returns -1

Because the dynamically generated functions aren't equal to each other, we won't be able to find them later in a sprite's behavior list if we want to remove it or check for duplicates. If we go back to the old world of using behavior names, all follow <target> behaviors equal each other so you can't have multiple versions of one behavior on the same sprite.

To handle this I'm storing behaviors as a an object with both a func and extraArgs. In this world the follow behavior looks like:

function follow(sprite, target) {
  sprite.moveToward(target);
}
function followBehavior(target) {
  return {
    func: follow,
    extraArgs: [target],
  };
}

When called in the draw loop, everything in extraArgs gets appended to the argument list. Two behaviors are equal if they use the same function and all their extraArgs are equal.

addBehavior still accepts a function if you don't need extraArgs.

[joshlory]

Does this mean follow(cat) will override follow(dog)? What if I want to follow both?

[balderdash]

My plan is for the behavior getter block to return an object with both func and extraArgs properties. In your example they won't be considered equal because they'll have different extraArgs, so you can add both behaviors.

This bit of code just makes addBehavior more convenient to use in helper library code if you're adding a behavior with no extra args.

[joshlory]

Extra?

[joshlory]

Now might a good time to start adding some basic test coverage of this behavior (hah). We don't need to test the whole file, but this method is a good starting place.

[balderdash]

I added a unit test, but had to pay the price of adding a terrible hack.

I eval the helper library so that the unit test doesn't have to run the whole JSInterpreter, but because tests are running in strict mode the variables defined in the helper library aren't made available in the calling scope. To get around this, the test first modifies the helper library to turn all of its global variable declarations into window properties (basically just running it through s/var /window./).

PTAL

[joshlory]

To get around this, the test first modifies the helper library to turn all of its global variable declarations into window properties (basically just running it through s/var /window./).

Hmm. Can we property export methods in GameLabJr.interpreted.js instead?

[balderdash]

I definitely need to put more thought into how we organize the helper library code. I imagine a pretty large chunk of it will be moved into common block snippets, and since that will mostly be written by levelbuilders I want to keep it pretty straightforward to write.

I think the ideal solution would be to leave the library as it is, and replace the hackery in this PR with a webpack loader that allows me to import its globals into test code as if they were properly exported. Some of the options on this wiki page look promising: https://github.com/webpack/docs/wiki/shimming-modules

Also, the globals need to be known to blockly so that it will work around them during codegen if you make a variable with the same name.

Also, it turns out that babel does process the helper library because the rule doesn't explicitly exclude *.interpreted.js:

code-dot-org/apps/webpack.js

Line 67 in 4331cb4

test: /\.jsx?$/,

That said, I think this is a problem for future Ram, so I'd like to merge this as-is.