Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable chaining of chai plugins #1617

Closed
wants to merge 6 commits into from
Closed

Enable chaining of chai plugins #1617

wants to merge 6 commits into from

Conversation

koddsson
Copy link
Member

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.

@koddsson koddsson requested a review from a team as a code owner May 14, 2024 09:38
@koddsson koddsson enabled auto-merge (squash) May 14, 2024 09:39
@43081j
Copy link
Contributor

43081j commented May 14, 2024

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
Copy link
Member Author

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
Copy link
Contributor

43081j commented May 14, 2024

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
Copy link
Member Author

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
Copy link
Contributor

43081j commented May 14, 2024

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
Copy link
Member Author

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
Copy link
Contributor

43081j commented May 14, 2024

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
Copy link
Contributor

43081j commented May 14, 2024

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

test/plugins.js Outdated Show resolved Hide resolved
Comment on lines +11 to +17
chai.assert.testing = 'successful';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

@koddsson
Copy link
Member Author

koddsson commented Sep 7, 2024

I'm cleaning up some old PRs and am gonna close this. I'll re-open a different one if I end up working on this.

@koddsson koddsson closed this Sep 7, 2024
auto-merge was automatically disabled September 7, 2024 13:13

Pull request was closed

@koddsson koddsson deleted the allow-chained-plugins branch September 7, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use() function only returns plugin first time
2 participants