Enable chaining of chai plugins

[koddsson]

This changes moves the responsibility of checking if a plugin has already been registered to the plugin itself.

I think this fixes #1603, if not I probably need a better test to make sure.

[43081j]

this would mean chai extensions (properties, methods, etc) get overwritten/added each time right?

do we know what our suggestion would be to plugin authors for ensuring they don't accidentally duplicate things?

just thinking of a situation where they add a method which wraps the original. so if you call it many times, you end up with customMethod(customMethod(originalMethod(...args))) basically (which would probably work in many cases but be not great)

[koddsson]

this would mean chai extensions (properties, methods, etc) get overwritten/added each time right?

I think so. Isn't that how it worked in chai@4 as well?

do we know what our suggestion would be to plugin authors for ensuring they don't accidentally duplicate things?

just thinking of a situation where they add a method which wraps the original. so if you call it many times, you end up with customMethod(customMethod(originalMethod(...args))) basically (which would probably work in many cases but be not great)

I'm probably missing something so maybe we can express this as a test? Something like:

function somePlugin(chai) {
  if (chai.Assertion.prototype.testing) return;

  Object.defineProperty(chai.Assertion.prototype, 'testing', {
    get: function () {
      return 'successful';
    },
  });
}

function someOtherPlugin(chai) {
  if (chai.Assertion.prototype.testing) return;

  Object.defineProperty(chai.Assertion.prototype, 'testing', {
    get: function () {
      return 'failed';
    },
  });
}

let chai = use(somePlugin);
chai = chai.use(someOtherPlugin);

expect(chai.myProperty).to.equal('successful');

[43081j]

in chai 4 it would've only called it once afaik (thats what used was for)

i may be talking nonsense but maybe someone does something like this:

chai.overwriteProperty('equal', function (_super) {
  if (something) {
    return _super.call(this);
  }
  return someOtherThing();
});

which presumably, when called many times in some cases, would result in _super(_super(_super())) etc

so it may be worth us documenting some recommended way of avoiding that, i.e. checking you already overwrote the property, added the method, etc

[koddsson]

You're probably right and it's something I haven't though of. Without a bit more concrete test I'm just not following 😅

I added the example test from your comment in 44af64d but I feel like I'm missing something obvious.

[43081j]

basically im not sure if overwriteProperty multiple times results in the original being super or the last overwritten one

given the example before:

chai.overwriteProperty('equal', function (_super) {
  if (something) {
    return _super.call(this);
  }
  return someOtherThing();
});

i wonder if _super is the current property, not the original

so if you did this:

const handler = function (_super) {
  console.log('Called!'); // log something out

  return _super.call(this);
};

chai.overwriteProperty('equal', handler);
chai.overwriteProperty('equal', handler); // equivalent to you calling `use` twice

chai.expect(123).to.equal(123);

does it log Called! twice or once?

im just saying, if it calls it twice, we should probably document a recommended way of avoiding registering the same thing twice

does it make sense now?

[koddsson]

does it make sense now?

I think I'm getting it. I was hoping to run some of the code so I could poke and prod and compare chai@4 and chai@5 but I can't get anything to work. Running this in chai@4 results in a error:

const chai = require("chai");

function somePlugin(chai) {
  chai.util.overwriteProperty(chai.Assertion.prototype, "equal", function (_super) {
    console.log("Called!"); // log something out

    console.log(_super);
    return _super.call(this);
  });
}

chai.use(somePlugin);
chai.use(somePlugin);

chai.expect(123).to.equal(123);

im just saying, if it calls it twice, we should probably document a recommended way of avoiding registering the same thing twice

So document things like https://github.com/chaijs/chai/pull/1617/files#diff-919c27039f4865fa252acfbd90d91c8b6c33b42dbf3d593fac7648cc6aa7a3f5R4 but maybe a bit more reliable? Otherwise we could also document that people should be vary of registering the same plugin twice as that can result in undefined behaviour?

[43081j]

i just tried it and it seems to call it once in both 5 and 4

its because its a method (equal), not a property:

import * as chai from 'chai';

function somePlugin(base) {
  base.util.overwriteMethod(base.Assertion.prototype, "equal", function (_super) {
    return function(...args) {
      console.log("Called!"); // log something out

      return _super.call(this, ...args);
    };
  });
}

chai.use(somePlugin);
chai.use(somePlugin);

chai.expect(123).to.equal(123);

so i think you can ignore me and all is fine

edit: nevermind, with your change it does call it twice. so we should probably document that as the plugin author's responsibility

[43081j]

basically this is what i'd tell authors to do (overwritten flag):

import * as chai from 'chai';

let overwritten = false;

function somePlugin(base) {
  if (!overwritten) {
    base.util.overwriteMethod(base.Assertion.prototype, "equal", function (_super) {
      return function(...args) {
        console.log("Called!"); // log something out

        return _super.call(this, ...args);
      };
    });
    overwritten = true;
  }
}

chai.use(somePlugin);
chai.use(somePlugin);

chai.expect(123).to.equal(123); // Logs `Called!` only once

[koddsson]

I thought it was surprising I had to do this. Unless I'm misunderstanding how this is supposed to work.

If it's correct that we need to define a property on both chai.assert and chai.Assertion.prototype I was thinking we could think about a more user friendly way to write plugins for the next breaking version.

[43081j]

you would normally use util.addProperty at least, like:

util.addProperty(Assertion.prototype, 'testing', function () {
  this.assert(this._obj === 'foo');
});

so expect and should will inherit that

but you're right you'd still have to manually extend the assert interface i think, something like:

assert.isTesting = function() {
  new Assertion(val).testing;
}