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

Copied typings from DefinitelyTyped #11

Merged
merged 1 commit into from
Jan 2, 2018
Merged

Copied typings from DefinitelyTyped #11

merged 1 commit into from
Jan 2, 2018

Conversation

JoshuaKGoldberg
Copy link
Contributor

Adds TypeScript as a dev dependency and specifies index.d.ts as typings, with --noEmit tests run as part of test.

Fixes #10.

@@ -1,27 +1,29 @@
{
"name": "assertion-error"
Copy link
Member

Choose a reason for hiding this comment

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

It feels so good to have this file indented like this 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is what happens automatically. I can't stand pre-line commas anyway! D:

@lucasfcosta
Copy link
Member

Thanks @JoshuaKGoldberg!

This looks awesome! LGTM.

Seems like we will have TypeDefinitions on our next release 😄

@demurgos
Copy link
Contributor

It's a good start.
It would be nice to capture the relation between the props argument the property available on the instances.

@demurgos
Copy link
Contributor

demurgos commented Nov 24, 2017

Here is a stricter modelization supporting the property types (I'm using an internal module declaration style because I was testing in my local project):

declare module "assertion-error" {
  type AssertionError<T> = Error & T & {showDiff: boolean};

  interface AssertionErrorConstructor {
    new<T>(message: string, props?: T, ssf?: Function): AssertionError<T>;
  }

  const AssertionError: AssertionErrorConstructor;

  export = AssertionError;
}

You can use it as:

const assertionError: AssertionError<{foo: number}> = new AssertionError("msg", {foo: 42});
const msg: string = assertionError.message;
const foo: number = assertionError.foo;

This brings more useful information about the assertion error (custom properties). Note: I used explicit types, but if you only use inference, it still properly detects that the instance has a property foo of type number.

Adds TypeScript as a dev dependency and specifies `index.d.ts` as typings, with `--noEmit` tests run as part of `test`.
@JoshuaKGoldberg
Copy link
Contributor Author

@demurgos added! I defaulted T to = {} because there might be usages of AssertionError out there that don't provide an explicit generic.

@demurgos
Copy link
Contributor

demurgos commented Dec 18, 2017

Thank you for taking time to update the typings.
It looks pretty good. I also added your branch as a dependency of one of my projects and experimented with it: everything works fine. No conflicts, support for require imports, ES imports, type inference for the properties, autocompletion. Good work.

@keithamus, @lucasfcosta
I'd recommend to merge it and publish it to npm so we can move forward with chaijs/chai#1092.

@demurgos
Copy link
Contributor

Could we move forward with this issue?

@keithamus keithamus merged commit e8b00f7 into chaijs:master Jan 2, 2018
@keithamus
Copy link
Member

Does someone want to raise a PR bumping the package.json so we can release?

@demurgos
Copy link
Contributor

demurgos commented Jan 2, 2018

I'll do a PR.
I think it should be semver minor bump because it's a feature for Typescript, but you may also consider it to be a patch because it has no runtime impact.

demurgos added a commit to demurgos/assertion-error that referenced this pull request Jan 2, 2018
- Add type definitions (chaijs#11)
@demurgos demurgos mentioned this pull request Jan 2, 2018
demurgos added a commit to demurgos/assertion-error that referenced this pull request Jan 2, 2018
- Add type definitions (chaijs#11)
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.

None yet

4 participants