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

Replace type.js with the "chaijs/type-detect" library #418

Closed
wants to merge 1 commit into from

Conversation

davelosert
Copy link
Member

Hi Guys,

i finally found the time to create this pull request, moving chai.js to use type-detect.

I had to change one minor line within assertions.assertThrows, as type-detect actually returns the type 'error' for a correctly inherhited Custom Error, not 'object'. Funny enough, this also fixes the other bug-issue i reported here.

You still want to be able to test thrown 'normal' Objects with a message-property for their messages? In this case, i would add another or-Condition like:

('object' === _.type(err) || 'error' === _.type(err)) && "message" in err

@davelosert davelosert force-pushed the Switch-To-Type-Detect branch 3 times, most recently from 3047904 to d262da2 Compare April 5, 2015 19:35
@keithamus
Copy link
Member

Looks good @Charminbear!

I'm a tiny bit concerned about the 'object'/'error' thing. Especially as I fear some users might have some code like expect(error).to.be.an('object'). This could be enough to push this change into v3.0.0, we don't want people upgrading chai and breaking tests (unless they upgrade to a new major version of course).

What are your thoughts?

@keithamus
Copy link
Member

Also @Charminbear could you please update the docs for an() with this PR.

https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js#L127-L148

It would be good to add some examples of how to detect new primitives, like Promises, typed arrays, DataViews etc, as well as an example of how to override Symbol.toStringTag to check your own types. Something like:

  /**
   * ### .a(type)
   *
   * The `a` and `an` assertions are aliases that can be
   * used either as language chains or to assert a value's
   * type.
   *
   *     // typeof
   *     expect('test').to.be.a('string');
   *     expect({ foo: 'bar' }).to.be.an('object');
   *     expect(null).to.be.a('null');
   *     expect(undefined).to.be.an('undefined');
   *     expect(new Promise).to.be.a('promise');
   *     expect(new Float32Array()).to.be.a('float32array');
   *
   *     // es6 overrides
   *     expect({[Symbol.toStringTag]:()=>'foo'}).to.be.a('foo');
   *
   *     // language chain
   *     expect(foo).to.be.an.instanceof(Foo);
   *
   * @name a
   * @alias an
   * @param {String} type
   * @param {String} message _optional_
   * @api public
   */

@davelosert
Copy link
Member Author

Hm, good Point @keithamus, i didnt even think that far. If we go strict by the semver rules this would be a breaking change, so yeah, i guess it should be in 3.0.0. Would also be the safer route to take as you said. Even though i think checking your error to be an object is not the most likely use-case, but I guess that's not the point. ;)
You want me to move this PR to the 3.x.x branch then?

Oh and will do the docs-update.

@keithamus
Copy link
Member

Looks great 😄. But yes I think it may be best as a breaking change - therefore if you could PR it against the 3.x.x branch that'd be swell.

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.

2 participants