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

Make assert work in oldIE #117

Closed
domenic opened this issue Dec 6, 2012 · 26 comments
Closed

Make assert work in oldIE #117

domenic opened this issue Dec 6, 2012 · 26 comments

Comments

@domenic
Copy link
Contributor

domenic commented Dec 6, 2012

The assert interface depends on the expect and should interfaces. This is a real shame because it stops us from working in IE<=8. We should make it work!

@logicalparadox
Copy link
Member

I agree, though admittedly, IE8 wasn't what put this topic on my radar initially. Unfortunately, the assert interface has always gotten less attention than expect and should interfaces because it was an afterthought. As such, all the plugins that have come out are "chainables". If we can work to fully separate the "core assertions" from the interfaces, and do so in such a way that they are "way-back" browser compatible, we can accomplish this IE requirement and make it easier for plugins to expose their features to multiple interfaces. Win-win.

I want to do this right, and a lot of internals will need to be shifted to accomplish this.

@rowanmanning
Copy link

I got burned by this the other day actually, glad to see it's been raised. I'm not familiar with the code-base (so I may be more of a hindrance than anything) but I'd be willing to help out with this if you need it.

@tmcw
Copy link

tmcw commented Jan 2, 2013

Current issues here are the use of Object.create (for which there's an easy polyfill) and Object.defineProperty (for which a polyfill is trickier to pull off)

@logicalparadox
Copy link
Member

Discussion of how to refactor the Plugin API for more flexibility is taking place on this google group thread. If implemented correctly, a glorious side-effect of this refactor would be an assert interface that is free from needing defineProperty support and could work in legacy IE.

Please head over to the Google Group and read up/discuss!

@nzakas
Copy link

nzakas commented Mar 19, 2014

Just curious if this is still forthcoming or not? Lack of oldIE support is the big things preventing us from adopting Chai.

@jokeyrhyme
Copy link

I have a solution that is working for me in IE8: https://github.com/jokeyrhyme/assertive-chai.js
I've just ripped off the Assert API, as it's the fancy Expect / Should hooks that seem to be where much of the ES5 is mandated.

@mightyiam
Copy link

+1

@mightyiam
Copy link

Please add a note about this in the readme or something?

@mightyiam
Copy link

OK. How hard is this, please?

Does anyone have a vague estimation of the amount of work this will take?

How deep does this go? Which files is this confined to? @keithamus?

I would like a go at this, please.

@mightyiam
Copy link

I see that if I desire a part in this I should participate in the above linked thread.

Thank you.

@wrumsby
Copy link

wrumsby commented Feb 18, 2015

@mightyiam have a look at ristretto - it provides a subset of the Chai API and we used it to run 1,000s of tests until we no longer needed to support older versions of IE.

@mightyiam
Copy link

Thank you @wrumsby.

I think that my likely candidates are Jasmine and Unexpected.

Since I desire to use Karma and Jasmine has already a plugin for Karma, I'm leaning towards it more.

Ristretto seems to be AMD, while I'm using CommonJS. I know that gap is fillable but, you know...

@mightyiam
Copy link

For those looking for an assertion library for IE < 9, proclaim, at least in my case, was a drop-in replacement for Chai's assert.

@jokeyrhyme
Copy link

@logicalparadox @vesln @domenic : would you be interested in a pull request that uses enricomarino/is (or something like it), dropping / extracting the comparison logic in https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js ? The Assert API could then be rewritten to use those Boolean-returning functions more directly, short-circuiting the setter/getter stuff that breaks in IE8.

@keithamus
Copy link
Member

@mightyiam sorry for not commenting earlier. I somehow missed this notifications.

I don't think this will be solved overnight - I was maybe a bit hasty to add the pull-request-wanted label. I think we should work towards specifying a design mentioned in @logicalparadox's comment https://groups.google.com/d/msg/chaijs/TXtkOAGyetU/tE0iC_WJxboJ.

@jokeyrhyme I'm not sure how enricomarino/is really helps us here. I'm also not sure what you mean about assert API returning Booleans. Once again, I'd suggest we pick up on Jake's comments.

I think the answer is a more expressive plugin API which automatically composes interfaces for both legacy and modern browsers. Potentially at this point we can look at splitting out the chai codebase into smaller modules to facilitate this - but the top priority is an expressive interface for plugin authors.

@jokeyrhyme
Copy link

@keithamus:

I'm also not sure what you mean about assert API returning Booleans.

You're misquoting me a little (or I've articulated my idea poorly). What I meant was essentially replace this (just an example):

assert.isTrue = function (value, msg) {
  if (value !== true) { throw new AssertionError(msg); }
}

... with this:

var is = require('is'); // either ennricomarico/is or a chaijs-owned alternative
assert.isTrue = function (value, msg) {
  if (!is.true(value)) { throw new AssertionError(msg); }
}

This separates the value-testing logic from the assertion throwing logic. Our problem right now is that the value tests are mixed in with code that will never work in IE8, and the assert API is built on top of that code. Once the value tests are separate, we can drop the expect/should layer in the middle, which will allow assert to function in more environments.

@jokeyrhyme
Copy link

@keithamus having read through Jake's comments, however, I'm not sure that my suggestion is entirely in line with his vision, although that post focuses on plugins and not the core API, so I don't really know that my suggestion is not applicable (yet).

@keithamus
Copy link
Member

@jokeyrhyme I see what you mean. I think the problem we have is that plugins are currently designed to be built to the expect/should api, and assert interface just wraps those. You make a good point about having a generic set of assertions for all interfaces to plug into - but I think this should be designed top-down for chai (and plugins) rather than bottom-up, if that makes any sense.

@keithamus
Copy link
Member

Just to get the ball rolling with a design - as I've stated I think the issue mostly revolves around how plugins are authored. I think a tweak to the plugin interface could provide us with the information needed to generate the various interfaces we have. Here is my thoughts put on paper:

function (chai, utils) {
    // All chai plugins must start by calling chai.plugin() with a namespace
    // string (can be blank).
    var plugin = chai.plugin('');

    // With the plugin interface, methods can be added using `assertions.add()`
    // which can also be prefixed with `withModifiers()`
    // `add()` works the same way as `addMethod`
    plugin.withModifiers('not', 'deep').add('equal', function (value) {
        return this.assert(
            flag(this, 'object') === value,
            'expected #{this} to equal #{exp}',
            'expected #{this} to not equal #{exp}',
            value
        );
    });
    // generates the following APIs (depending on which interface users choose):
    //
    // - expect
    //   expect(foo).to.equal('bar');
    //   expect(foo).to.not.equal('bar');
    //   expect(foo).to.deep.equal('bar');
    //   expect(foo).to.not.deep.equal('bar');
    //
    // - expect legacy
    //   expect(foo).toEqual('bar');
    //   expect(foo).toNotEqual('bar');
    //   expect(foo).toDeepEqual('bar');
    //   expect(foo).toNotDeepEqual('bar');
    //
    // - should
    //   foo.should.equal('bar');
    //   foo.should.deep.equal('bar');
    //   foo.should.not.equal('bar');
    //   foo.should.not.deep.equal('bar');
    //
    // - assert
    //   assert.equal(foo, 'bar');
    //   assert.not.equal(foo, 'bar');
    //   assert.deep.equal(foo, 'bar');
    //   assert.not.deep.equal(foo, 'bar');
    //
    // - assert legacy
    //   assert.equal(foo, 'bar');
    //   assert.notEqual(foo, 'bar');
    //   assert.deepEqual(foo, 'bar');
    //   assert.notDeepEqual(foo, 'bar');

    // `addProperty` can also be used, and also be chained from `withModifiers`
    // The method name is expressed as dot-notation (`'be.ok`'), this is
    // useful in two ways:
    //  - Fluent interfaces, such as expect and should, can add the `be` keyword
    //    as chainable grammar (properties that do nothing), automagically.
    //  - Legacy interfaces, such as "expect legacy" can use these to camelcase
    //    into a single method name that still makes sense
    //
    // Notice that this does not have the `.deep` modifier - and so we can issue
    // warnings when it is called with `.deep` as its not a registered modifier
    // for this assertion.
    plugin.withModifiers('not').addProperty('be.ok', function () {
        return this.assert(
            Boolean(flag(this, 'object')),
            'expected #{this} to be ok',
            'expected #{this} to not be ok',
            value
        );
    });
    // generates the following APIs (depending on which interface users choose):
    //
    // - expect
    //   expect(foo).to.be.ok;
    //   expect(foo).to.not.be.ok;
    //
    // - expect legacy
    //   expect(foo).toBeOk();
    //   expect(foo).toNotBeOk();
    //
    // - should
    //   foo.should.be.ok;
    //   foo.should.not.be.ok;
    //
    // - assert
    //   assert.ok(foo);
    //   assert.not.ok(foo, 'bar');
    //
    // - assert legacy
    //   assert.ok(foo, 'bar');
    //   assert.notOk(foo, 'bar');
}

For more complex plugins, such as chai-spies, the following can be done:

function (chai, utils) {
    // A namespace is given specifically to make methods less ambiguous
    var plugin = chai.plugin('spy');

    // Once again, grammar is added to help with method names.
    plugin.withModifiers('not').add('be.calledWith', function () {
        //...
    });
    // generates the following APIs (depending on which interface users choose):
    //
    // - expect
    //   expect(foo).to.be.calledWith('bar');
    //   expect(foo).to.not.be.calledWith('bar');
    //
    // - expect legacy
    //   expect(foo).toBeCalledWith('bar');
    //   expect(foo).toNotBeCalledWith('bar');
    //
    // - should
    //   foo.should.be.calledWith('bar');
    //   foo.should.not.be.calledWith('bar');
    //
    // - assert
    //   assert.spy.calledWith(foo, 'bar');
    //   assert.spy.not.calledWith(foo, 'bar');
    //
    // - assert legacy
    //   assert.spyCalledWith(foo, 'bar');
    //   assert.spyNotCaledWith(foo, 'bar');

    // More grammar is added to help with method names.
    // imperative('isSpy') is used, because the auto generated method name would
    // be `spy.spy` (for assert) or `spySpy` (for legacy assert) - so using
    //`imperative()` we can specifically tune the method name for those
    // interfaces:
    plugin.withModifiers('not').imperative('isSpy').addProperty('be.a.spy', function () {
        //...
    });
    // generates the following APIs (depending on which interface users choose):
    //
    // - expect
    //   expect(foo).to.be.a.spy;
    //   expect(foo).to.not.be.a.spy;
    //
    // - expect legacy
    //   expect(foo).toBeASpy();
    //   expect(foo).toNotBeASpy();
    //
    // - should
    //   foo.should.be.a.spy;
    //   foo.should.not.be.a.spy;
    //
    // - assert
    //   assert.spy.isSpy(foo, 'bar');
    //   assert.spy.isSpy(foo, 'bar');
    //
    // - assert legacy
    //   assert.spyIsSpy(foo, 'bar');
    //   assert.spyIsSpy(foo, 'bar');
}

Please dissect and bike-shed this as much as possible, if its a bad idea I'd definitely like to know, I feel it could be a step in the right direction but may be off of the mark here. If you author plugins I'd definitely like to hear from you.

@electricmonk
Copy link

@keithamus what's missing for me here is the means for a plugin to declare what types of object it can assert. Something like what I did here with the predicate.

@keithamus
Copy link
Member

@electricmonk I think what you've done with chai-react-element is pretty much what I'd like to see. It seems like its a pain, but actually the power behind having no Chai API for this kind of thing is that you just write your own code, code that you understand, code that you don't need to read docs for. If we just lean on standard JS conventions rather than a public API, I think the (small) barrier to writing chai plugins falls away to nothing.

@mgol
Copy link

mgol commented Jan 5, 2016

@electricmonk Could you press y on the keyboard before selecting the link & copying it? master will be changed to its commit hash. Otherwise your comment will become useless to others as soon as you modify this file, adding or removing lines.

@keithamus
Copy link
Member

@mgol TIL how to do that 😄

@electricmonk
Copy link

@mgol done
@keithamus - thing is, the API you described above doesn't mention any place for declaring a predicate for which the plugin is applicable, nor does it mention how does a plugin call 'next', so it (as far as I understand) represents a rewrite of addProperty / addMethod and not taking into account overwriteMethod. What am I missing?

@keithamus
Copy link
Member

@electricmonk I have made #585 to further discuss a new syntax. In that issue I have an example of a newer syntax which I think is the right fit for a simple, convention-over-code API that chai could adopt. It also addresses what you mention about overwriteMethod (by adding interceptors). Please can we continue the discussion there?

@keithamus
Copy link
Member

Okay, I think it's time to close this issue. It's been a fun ride everyone, but we've settled on a stance of supporting IE9+, and older browsers are unlikely to be supported now. Depending on how things go, especially with #585 we might (slim but possible chance) revisit the support matrix - but time heals all wounds (where wounds are old versions of IE) right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests