testdouble / testdouble.js

A minimal test double library for TDD with JavaScript
MIT License
1.42k stars 142 forks source link

TypeScript: Improve Definition #238

Open sgtoj opened 7 years ago

sgtoj commented 7 years ago

List of improvements I believe is needed for the TypeScript definition. I have already started working on this list. Love to hear feedback if you have any.

Changes: sgtoj/testdouble.js:improve-ts

searls commented 7 years ago

Any improvements are appreciated. Hey @duluca, you'd suggested helping out with our typescript story, want to look at this?

sgtoj commented 7 years ago

Updated OP with the link to the fork and branch.

sgtoj commented 7 years ago

PR #239

duluca commented 7 years ago

Will review. @sgtoj please lmk if there's anything specific I can help with. I'll leave any comments re: code on the pr

sgtoj commented 7 years ago

Disclaimer: I am not the best when it comes to communication so there may be the grammar mistakes or typos. I did a quick proof reading before committing.

The jsdoc comments probably could use some additions or improvements from someone who has more experience with the framework and/or understanding behind the abstractions. Otherwise, it should be ready.

duluca commented 7 years ago

Sounds good. Will give it a shake tonight.

albertogasparin commented 7 years ago

Sorry to jump in, but with published typings I'm getting an error in VSCode (Typescript 2.3):

const myMock = td.object(['run', 'search']);
td.when(myMock.run()).thenCallback(null);
// Error: Property 'run' does not exist on type 'DoubledObjectWithKey<"run" | "search">'.

Is it something this PR is going to solve? Or are you guys not getting this?

searls commented 7 years ago

@albertogasparin I suggest you attempt checking out the branch in #239 to find out for yourself

albertogasparin commented 7 years ago

Yep, was doing it. In fact, I'm still getting the same error even with the PR typings. Unfortunately I don't have much experience with Typescript, so I'm unable propose a fix

sgtoj commented 7 years ago

@albertogasparin I will look in to this issue. Hopefully, I will find a fix, add a test, and submit a new PR.

sgtoj commented 7 years ago

I fixed the issue and added a new TS test in #239. @albertogasparin @searls

Example

albertogasparin commented 7 years ago

@sgtoj Worked like a charm, thanks! Hope to see #239 merged soon 😉

searls commented 7 years ago

Good to hear. I'm chatting with @duluca tomorrow and want to have a chat with him about our TS approach more broadly before merging

SimonSchick commented 7 years ago

Any updates on this?

sgtoj commented 7 years ago

Let me know if there is anything I forgot to do or address.

duluca commented 7 years ago

@sgtoj as noted on PR #239, there are a couple of tasks due: TODO: [ ] Update from master, resolve conflicts [ ] Update PR and make sure CI checks pass

sgtoj commented 7 years ago

@duluca I believe we are ready.

duluca commented 7 years ago

Close, but no cigar. The test code must live in the regression/test.ts file.