testdouble.js icon indicating copy to clipboard operation
testdouble.js copied to clipboard

TypeScript: Improve Definition

Open sgtoj opened this issue 7 years ago • 18 comments

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.

  • [x] Organize / Restructure
  • [x] Add JSDoc for all functions
  • [x] Define td.func()
  • [x] Add generic types to td.function() and td.func()
  • [x] Define td.constructor()
  • [x] Add to ./test/src/typescript/test.ts to reflect any additions.

Changes: sgtoj/testdouble.js:improve-ts

sgtoj avatar May 04 '17 13:05 sgtoj

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

searls avatar May 04 '17 13:05 searls

Updated OP with the link to the fork and branch.

sgtoj avatar May 04 '17 14:05 sgtoj

PR #239

sgtoj avatar May 04 '17 14:05 sgtoj

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

duluca avatar May 04 '17 15:05 duluca

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.

sgtoj avatar May 04 '17 15:05 sgtoj

Sounds good. Will give it a shake tonight.

duluca avatar May 04 '17 15:05 duluca

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?

albertogasparin avatar May 09 '17 11:05 albertogasparin

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

searls avatar May 09 '17 11:05 searls

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

albertogasparin avatar May 09 '17 13:05 albertogasparin

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

sgtoj avatar May 11 '17 01:05 sgtoj

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

Example

sgtoj avatar May 11 '17 02:05 sgtoj

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

albertogasparin avatar May 11 '17 08:05 albertogasparin

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

searls avatar May 11 '17 11:05 searls

Any updates on this?

SimonSchick avatar Sep 09 '17 02:09 SimonSchick

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

sgtoj avatar Sep 13 '17 13:09 sgtoj

@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

duluca avatar Sep 13 '17 14:09 duluca

@duluca I believe we are ready.

sgtoj avatar Sep 15 '17 17:09 sgtoj

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

duluca avatar Sep 15 '17 18:09 duluca