Open koddsson opened 3 months ago
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)
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');
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
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.
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?
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?
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
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
This changes moves the responsibility of checking if a plugin has already been registered to the plugin itself.
I think this fixes https://github.com/chaijs/chai/issues/1603, if not I probably need a better test to make sure.