Use ES6 classes

[Zielak]

I couldn't wait so I tried it myself. I only changed how classes are defined. All build and testing tools are not changed.

3

[danielepolencic]

This looks great!

Let me review it and I'll soon merge it in!

💪 💪 💪

[Zielak]

Thanks for CR, updated :)

[danielepolencic]

I'd like to merge this PR, but I think there're still cases where we are bitten by having properties on the prototype. To explain what I mean, have a look a the following example:

function myClass(){}
myClass.prototype = {
    settings: {name: 'random'}
};

m1 = new myClass();
m2 = new myClass();

console.log(m1.settings.name) // 'random'
console.log(m2.settings.name) // 'random'

m1.settings.name = 'Dan'

console.log(m1.settings.name) // 'Dan'
console.log(m2.settings.name) // 'Dan' <- You might have expected 'random'

We're doing something similar here: https://github.com/behavior3/behavior3js/pull/30/files#diff-1cfa0cb7bc40f0bcf44081f319b96a43R127 . The array is shared by reference. The following line is similar.

I understand that having a description of the property is convenient, but perhaps we could use something like this? https://github.com/jsdoc3/jsdoc/issues/819

I'd like to avoid promoting practices that are very hard to debug and have an odd behaviour. Properties on the prototype is one of them.

What do you think?

[Zielak]

I had to spend a minute on that example and now I see it. I think I've just gotten used to the current code with the prototype properties and their descriptions. I'll get rid of those nightmares and move descriptions to constructor. Sadly, I don't see that jsdoc supports that kind of naming with #

/**
 * The actor's name.
 * @member Actor#name
 */
this.name = "leo";

but hopefully it'll change in the future.

[Zielak]

Also, this change will require to change some tests:

test('Prototype', function() {
    assert.equal(Error.prototype.name, 'Error');
});

[Zielak]

Another thing just popped up to my mind, at least for some properties:

class Error extends Action {
  // ...
  /**
   * Node name. Default to `Error`.
   * 
   * @readonly
   * @memberof Error
   */
  get name(){
    return 'Error';
  }
}

category will get inherited and name will get overriden when extending:

class Action {
    get name(){
        return 'Action';
    }
    get category(){
        return 'ACTIONCATEGORY';
    }
}

class Error extends Action {
    get name(){
        return 'Error';
    }
}

var e = new Error();
`e.name: ${e.name}, e.category: ${e.category}`
=> "e.name: Error, e.category: ACTIONCATEGORY"

[Zielak]

With my last commit I changed only actions for now (time to sleep). Take a look at Wait.js, it has name and title but also parameters, which I renamed to properties, as parameters are deprecated (found that note on BaseNode.js). I'm not sure if we should keep properties read-only as I don't know the real usage of it. What do you think?

[danielepolencic]

I think name, categories, etc. are used by the visual editor. What if we slightly tweak the code like this: take Action as an example. The new action is:

export default class Action extends BaseNode {
  constructor({name}) {
    this.category = ACTION;
    this.name = name;
  }
};

We could have Error as:

export default class Error extends Action {
  constructor() {
     super({name: 'Error'});
  }
  tick() {/* ... */ }
}

I'm not worried about JSDoc, tbh.

[Zielak]

Hey, my changes are ready.

This makes it so milliseconds has default value of 0 if not present, and it doesn't break when there's nothing in constructor's arguments.

constructor({milliseconds = 0} = {}) {

Happy weekend!

[danielepolencic]

Great effort 💪