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

embracing proxies #787

Closed
yairper opened this issue Sep 7, 2016 · 8 comments
Closed

embracing proxies #787

yairper opened this issue Sep 7, 2016 · 8 comments

Comments

@yairper
Copy link

yairper commented Sep 7, 2016

Hi,

We could start enjoying the power of proxies to create even better DSL.
Examples:

let element = { visible: true }
element.should.be.visible
let element = { isVisible: true }
element.should.be.visible
let response = { status: 200 }
response.should.have.status(200)

WDYT?

@meeber
Copy link
Contributor

meeber commented Sep 7, 2016

My first impression is that it sounds cool in theory but in practice it opens the door too wide for mistakes and unexpected behavior without adding enough benefit over .property(name, value). As discussed in
#726, one of our central focuses is making it difficult for developers to accidentally write tests that look and act as if they're correct but actually silently aren't.

I think enabling cool shortcuts like .be.visible is better served via domain-specific plugins as opposed to enabled via a proxy-based catch-all.

@yairper
Copy link
Author

yairper commented Sep 7, 2016

I've seen that in v4 proxies are used to protect from such problems to occur,
wouldn't it be enough to change it to something like
verifyChaiPropertyExists() || verifyTargetPropertyExists() ?

@yairper
Copy link
Author

yairper commented Sep 7, 2016

  • If we wanna be extra safe, we could flag be/have so we could restrict which assertion to use when calling visible/status/etc...

@meeber
Copy link
Contributor

meeber commented Sep 7, 2016

The use of proxy in 4.x is for defensive purposes only.

Acting as a catch-all property-assertion in the proposed way, I think there's too much room for accidents and misinterpretation.

let gasTank = {
  empty: true,
};

gasTank.should.be.empty;

Did the developer mean to check the domain-specific concept of a tank of gas being empty, or did they mean to use Chai's empty assertion? Maybe the developer knows and got it right, maybe they didn't. Maybe the developer's coworker reviewing the code knows and understands what's being tested, maybe they don't.

This becomes even more problematic when you consider how the behavior changes between environments that support proxies and those that don't.

let element = { visible: true }
element.should.be.visible

In proxy-supported environments, this would perform an assertion to check if element.visible was set to true or not. In non-proxy supported environments, it would just silently pass. To be encouraging a construct that has such dangerously varying behavior between JS engines is a non-starter. The whole point of the proxy stuff in v4 is to at least give developers a chance to detect a meaningless assertion, to reduce risk and error.

Due to the risk, I think this type of change is better suited to a plugin as opposed to Chai's default behavior. I would recommend that such a plugin be configured to only work in environments that support proxies.

Restricting the behavior to .be and .have doesn't alleviate my concerns.

@yairper yairper closed this as completed Sep 7, 2016
@yairper
Copy link
Author

yairper commented Sep 7, 2016

Could such plugin (which monkey patches Chai) be exposed at chaijs.com/plugins ?

@meeber
Copy link
Contributor

meeber commented Sep 7, 2016

@yairper I suspect it's possible to write a normal plugin (http://chaijs.com/guide/plugins/) that implements the desired behavior by replacing be and have with custom logic. Adding chai-plugin keyword to the plugin's package.json would cause it to be added to the website list.

If it turns out there's really no way to implement this behavior through the plugin system and forking Chai is the only option, then I'm not sure what the best way to advertise it is. @keithamus @lucasfcosta?

@yairper
Copy link
Author

yairper commented Sep 7, 2016

I checked it up, and it could be done using the conventional way, Thanks !

@lucasfcosta
Copy link
Member

lucasfcosta commented Sep 7, 2016

@meeber Hmm, I'm not sure this behavior cannot be added to Chai through the plugin system.
Since chai.use basically gives the plugin developer the whole object to do whatever they please with I think it would be possible to implement this. It would be a lot of work probably, but I think it is possible.

Regarding the addition of this to the Core I totally agree with @meeber. His great explanation covers every thought I've had about it. I'd rather keep this in a separate plugin and avoid adding the possibility of writing contradictory assertions.

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

No branches or pull requests

3 participants