-
-
Notifications
You must be signed in to change notification settings - Fork 697
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
Refactor plugin declarations #585
Comments
As I wrote in #528 and #467, one major problem in my opinion is sharing namespaces, for instance two plugins that define a Having said that, I think that the new API should allow declaring a predicate for whether this plugin is applicable for the object under assertion: export default {
assertions: {
'depth': {
'predicate': obj => obj instanceof SwimingPool
'assert': {...}
}
}
} |
The intent with the above examples was that
Having said that, I like the idea of having a predicate based assertions - but I wonder if that is perhaps something that becomes difficult to debug:
|
I'm afraid that the way you described the API might be a bit too loose, so that people (like I just did!) could miss it altogether!
Well, I expect that an exception would be thrown. What happens in the solution for #467?
Good question. What did you imagine in your design? what you're doing, as I understand, is kind of a chain of responsibility pattern. How would it deal with a plugin with no predicate? with a conflict? seems like it's a matter of sorting the plugins, so that the first plugin registered wins.
|
An exception should be thrown - but what kind of exception? How can we surface this information to the user in a way that they would understand without being a plugin author.
The design I have above skirts over this issue by making the author manually override methods. Not saying this is better or worse.
The problem here is that we're pushing the problem onto the user, meaning more support requests/bug reports, a requirement for better docs for every plugin. It could potentially be a big problem. Ideally we should have a solution which does not depend on fragile things like plugin order. |
Eventually this is probably your decision, right? :) I think that one of the things that really hinders progress here is the fact that In both, additional matchers (plugins) are just functions that are passed to an assertion method, so that the only chance for a conflict would be the user actively importing two matchers with the same name - in which case the file will not compile / run. |
Hopefully not. I'm not a plugin author, nor a BDFL. I simply have the benefit of being the consumer of all issues meaning I can specify requirements for improving our plugin system. The reason this issue exists is for active bikeshedding by plugin authors so that they're getting the simplest system for writing plugins (that falls inline with some of the major issues chai faces).
This is definitely an option. I have considered looking at enforcing name-spacing of plugins, so that conflicts can't occur, but I've steered away from that for a few reasons:
I suppose we could try to implement some kind of trait based system similar to hamcrest, it doesn't differ that much from the predicate system you proposed above, with similar pitfals. Ultimately everything will have upsides and downsides, our rubric is:
|
Ok. How do we make sure that all plugin authors know that this issue exists and can put in their 2 cents? |
I have, in the past, @mentioned all plugin authors for sweeping chai changes (e.g. for migrating the chai plugins page), but thats kind of the nuclear option. |
@vesln and @marcodejongh as you've experienced some problems with the existing plugin architecture (enzymejs/chai-enzyme#11) I was hoping I could get your thoughts and opinions on the ideas expressed here. |
@keithamus yep! i'm going to go through the proposal this weekend and share feedback |
Awesome thanks! |
Hey @vesln did you manage to get any time to look at this? I'd be really interested in hearing your feedback. |
@electricmonk I think having a |
@keithamus can you please elaborate with a usage example? |
@electricmonk it'd be the same as you suggested here but the I think the way we express predicates could be slightly different though - I think export default {
assertions: {
// This would throw an error, as it has no `predicate` key
'equal': {
'assert': (actual, expected) => actual === expected
},
// This would throw an error, as `predicate.length` does not match `assert.length`
'equal': {
'predicate': [Object],
'assert': (actual, expected) => actual === expected
},
'equal': {
// This predicate will match `actual instanceof Object` and `expected instanceof Object`
'predicate': [ Object, Object ]
'assert': (actual, expected) => actual === expected
},
'equal': {
// This predicate will match `React.isElement(actual)` and `React.isElement(expected)`
'predicate': [ React.isElement, React.isElement ]
'assert': (actual, expected) => myReactEqlAlgo(actual, expected)
}
}
} (Having said all of that, I'd like to simplify the name |
why do we need a predicate against the Am I making sense? |
You're making sense, but I think there's maybe more utility in checking all params including export default {
assertions: {
'an': {
'predicate': [Object, String]
'assert': (actual, expected) => typeof actual === expected
},
'an': {
'predicate': [Object, Function]
'assert': (actual, expected) => actual instanceof expected
},
}
} This could be an |
fair enough. |
@keithamus sorry for getting back to you that late, it's been a bit intense over here. the proposal looks great! awesome work! one thing I want to also focus on is the ability of modules like i have not put too much thought into this, so i know i'm describing it super vaguely. i hope things normalize soon so i can jump in and we start the road to the next major version. ps. i'm super impressed with your contributions so far, hats down! |
Is this the reason why calling |
Hi @Alhadis, thanks for your question. When using Please let me know if I misunderstood your doubt or if you want to know anything else 😄 |
Ah right. No no, you understood my doubt correctly. :) Thanks for the speedy response! You guys rock. |
I like this proposal too :) Making the assertion chain context aware will be nice. |
@TiraO mentioned over in #1097
|
As we've been discussing a lot recently, we plan to make plugins a first class citizen for Chai v5 and make it easy for everyone to add plugins with assertions, modifiers and all that as easy as it is to add those things to the main library. Due to house-cleaning purposes, I'll be closing this issue for now, but we'll definitely use this issue as reference and take into account what's been discussed here when implementing it. |
I'm going to put this document here, as the start of a design discussion around how plugins could be written for a future version of chai. Similar discussions have been had before, over in Google Groups, and issues #117 and #457; but I wanted to start a new issue which can focus on the design of a new plugin architecture, and hopefully get some feedback.
Current Implementation
So, just to get everyone on track - right now, you can make a chai plugin by writing a little bit of code, like this:
Motivation
Right now we have a nicely expressive interface for creating plugins - but it has a few problems, which can be addressed by making working on a new interface. The problems it has are:
expect
andshould
interfaces as first class, but does not create methods for theassert
interface.addMethod
,addProperty
, andaddChainableMethod
. These in themselves aren't bad, but it does create a bit of an inconsistent interface, as it is an opt-in convention to use property assertions and chainable methods. Similarly, for users who only want to use method assertions, they have to use plugins like dirty-chai.expect(true).to.equal(true, 'True should be true!')
). Some plugins dont do this.eventually
to every method, which means programmatically rewriting every function. We should have an expressive syntax for this.not
. In fact,not
is so special that every assertion is required to support it. This should not be the case.chai.Assertion.addMethod
passes athis
value, and athis.assert
must be called within an assertion, to assert on anything. This could be simplified, and made less dependant on magic values likethis
.this.assert
has a list of ambiguous arguments, including boolean flags, and so it can be hard to decipher what is actually happening. If you don't passtrue
as the 6th argument - a diff wont show up for the assertion. This is a gotcha for even some of our best plugin authors.Requirements
Given the issues, we can set out a list of requirements that a new architecture should provide:
Must haves
addMethod
/addProperty
/addChainableMethod
functions.addMethod
should be able to be implemented much simpler - most small methods probably be distilled down to returning true/false. Making them "pure functions" (arguments are the only input, we determine what to do on the output, no side effects) would be an ideal.Nice to haves
expected {object} to {some expectation} but got {value}
- perhaps we could just have assertions declare the{some expectation}
part. We can generate error messages based on the actual keyword chain used for the assertion (e.g.expect(1).to.equal(2)
can beError: expected 1 to equal 2
).Draft concept
To start really thinking about a design - here is a draft concept I think could work when defining new assertions (this is an iteration on a previous draft in #117
Please note:
The text was updated successfully, but these errors were encountered: