Copied typings from DefinitelyTyped

[JoshuaKGoldberg]

Included them as lib/chai.d.ts, with --noEmit tests run in Makefile against test/typings/*.ts. [email protected] is now a dev dependency.

Closes https://github.com/chaijs/chai#650 and https://github.com/types/npm-chai#14.

I intentionally copy & pasted what's now used under @types/chai despite it being a little... sloppy in some parts (why the Resharper comment??). Improving the typings would be great as followup issues IMO.

Non-merge-conflict version of #822.

(note: I don't have make locally, so I'm using AppVeyor/Travis as my build machine. Ignore the force pushes until it works!) AppVeyor failures look unrelated. Can this get merged anyway? 😄

[codecov]

Codecov Report

Merging #1092 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1092   +/-   ##
=======================================
  Coverage   93.61%   93.61%           
=======================================
  Files          32       32           
  Lines        1628     1628           
  Branches      393      393           
=======================================
  Hits         1524     1524           
  Misses        104      104

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c1a99f...24d0aea. Read the comment docs.

[keithamus]

👍 this looks like a great start on this, and that the tests are passing, this seems like a good opportunity to merge this in and iterate from here.

🎉

[demurgos]

@keithamus
This looks like a good start effectively, but I am not sure if it is safe to merge yet due to the type reference to assertion-error.

[demurgos]

Looks good but I'd like some clarification around the use of the type reference. Packaging broken definitions is worse than no definitions.

[demurgos]

How does it deal with this type reference? Does it forces the user to install the types for assertion-error itself? This may pose issues due to versions mismatch between the version actually used by chai and the one uplifted to the consumer project.

[lucasfcosta]

Can't we bundle an specific version of this type definition to be used with Chai? This way we could always ensure it would be right (instead of depending on peer-deps).

(I don't understand much about TypeScript, so please let me know if I misunderstood what you've just said)

[demurgos]

The link you posted refers to the types for the typings tool. This tool was used before Typescript 2, because typescript did not support types from npm. These repos are currently (slowly) maintained for older tools but the recommended way is to publish the types to the library's repository. If this is impossible, the solution is to use DefinitelyTyped. Definitions in this repo are automatically published to the @types npm organization.

There are two solutions for the current issue:

• Adding type definitions to assertion-error (better)
• adding @types/assertion-error as a normal dependency to package.json (quicker)

[lucasfcosta]

The first option seems more adequate indeed.

As I've seen in the assertion-error repo, it's not that big, so it might be a simple issue to attack.

[JoshuaKGoldberg]

Oh, I had a note to deal with this and forgot about it...

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/assertion-error/index.d.ts

+1 to adding type definitions to assertion-error and -1 to adding any extra dependencies to package.json. I'll tackle that now.

[JoshuaKGoldberg]

Actually, just looking through this code, it looks like AssertionError is manually copy & pasted below. The comment above is // instead of /// and so doesn't do anything. I'll remove it.

[JoshuaKGoldberg]

@demurgos I've removed the reference... but looks like coverage is calculated as decreased on the PR. I don't really know how to deal with that :(

[demurgos]

Thanks for update. I checked the assertion-error typings and they seem good to merge. I'll take a look at this PR now.

Regarding coverage, I don't see any change: https://codecov.io/gh/chaijs/chai/pull/1092 Is it the right link? (Or do you only see the reduction in local?)

[JoshuaKGoldberg]

Huh, I was seeing a coveralls integration saying it was reduced. Never mind!

[demurgos]

I am having trouble to run the tests in your branch. Do you just use npm install && test-typescript? (module chai not found)

Edit: Solved, but I don't have much time right now. I'll submit a review later.

[demurgos]

Hi,
I added some commits on top of your branch: https://github.com/demurgos/chai/commits/types.

Here are the main changes:

• Fix the Typescript tests: solve type definitions resolution and global namespace modification (should style)
• Add TS options enforcing strict rules
• Reexport AssertionError from assertion-error: This currently relies on the unpublished JoshuaKGoldberg/assertion-error#types branch. The new version of assertion-error must first be published to npm. See Copied typings from DefinitelyTyped assertion-error#11

Chai uses strict commonJS (the exported object is just a namespace, not a function) so ideally the type definitions should avoid the export = pattern (it's just a hack, with some drawbacks related to interop) but this is a lower priority issue that could be fixed in a subsequent PR. The exports should simply be defined as ES exports: (export const assert: AssertStatic)

[demurgos]

The types for assertion-error were merged and the version was bumped. We're waiting for the new version to be published to npm but it should happen soon.

I think I'll update my branch and send a new PR superseding this one once the types are on npm. (See previous message)

[meeber]

I don't have any experience with TypeScript, and don't have the time to get smart on it, so I'll defer to other contributors and maintainers on this PR. Question though: What does merging this PR mean in terms of future changes to Chai? In particular, do all future new assertions and changes to existing assertions require corresponding updates to these new TypeScript-specific files before they can be safely merged?

[demurgos]

Additions to the Javascript code will need to be documented in the type definitions to be usable from Typescript. It has no runtime impact on Javascript users.

You have two ways to approach this:

• Require type definitions to always document the current JS code (enforce synchronization)
• Wait for Typescript users to send PRs to update the types when they need a feature.

You can also have a middle-ground were you declare methods and functions with the any type and let PRs refine the types.

The reason behind the addition of the types was that formalizing the documentation as types is valuable and having it here would help to have control over it and keep it in sync. Comment that prompted the PR. I also explained my reasons in this plugin-error issue.

[meeber]

What I'm most concerned about is breaking changes to existing assertions. If a breaking change is made to an existing assertion, is there an expectation that the type definitions need to be synchronized prior to the next semver major release, or is it generally understood and accepted by the community that such synchronization can happen in a patch after the major release?

[demurgos]

Right, I mainly addressed feature additions but not breaking changes.
What I am describing below are trade-offs that violate the purpose of types or semver in favor of easier maintenance. I don't think desynced types should be the norm but I will try to provide you the best description:

• Incomplete types are the less harmful: if a function is not defined, TS users cannot use it but they'll just send a PR, they can bypass type-checking in the meantime. This happens when a feature is added and the types are not updated.
• Types with extra symbols because of removal in the code without updating the types are more misleading. Still, they are usually found rather quickly and consumers can simply avoid to use them until the types are updated. This may be tolerated.
• Invalid types due to changes in the function signatures or types of exported symbols are the worst and should be avoided. If a function requires a string argument but is changed to accept a number argument then TS users will have to manually bypass typechecking around this function to use it. This is usually not tolerated.

These are the same trade-offs as for documentation: incomplete documentation is tolerable, wrong documentation is harmful. (That's why I call and treat it just as a more formal documentation).

---

From a practical point of view, the goal of types is to catch some errors before running the code. Because of this, I'd say that updating the types when breaking changes occur is when it's the most important. It's a great help for migration: they become very valuable because they allow to immediately find the lines that need to be updated (or most of them).

[demurgos]

I updated my branch to use the new assertion-error version. I submitted a new PR where we can continue the discussion (based on this PR, with fixes and changes discussed above).