Improve TS Support #239

sgtoj · 2017-05-04T14:57:54Z

See #238 for more information.

duluca · 2017-05-05T01:56:51Z

@sgtoj I've started the review, this may be more productive if I sent you a PR to fix some of the grammar stuff, which will also give me a chance run the code and look at how things are fitting together. I'm getting ready for a conference talk for tomorrow. So expect to hear from me again sometime Saturday!

duluca · 2017-05-08T18:30:44Z

@sgtoj @searls not sure if my testing methodology is correct here, but i'm running into an error. I tried to execute test/src/typescript/test.ts using node after running tsc on it to transpile it to a .js file.

Here's the error message that gets thrown when const bear = td.object<Dog>("Bear"); executes


:43:4)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)

I then attempted to dig into the object<T> declaration and lost the trail with export type DoubledObject<Subject> = Subject; I'm unable to find where Subject is defined.

I'm still getting oriented with the code base. Some help and direction would be appreciated.

sgtoj · 2017-05-10T23:12:12Z

@duluca Subject is defined as the generic parameter. So...

(DoubledObject<Subject> = Subject) === (DoubledObject<T> = T)

sgtoj · 2017-05-10T23:15:49Z

@duluca I would prefer to use T, though.

This reverts commit 0574e3e.

duluca · 2017-05-12T13:57:47Z

@sgtoj Ah I see, naming generics other than T is usually then when you limit what types can be used by the generic. If you can use any object, T or TObject makes sense. If say, the object must implement an interface called IDocument, then calling it TDocument helps the user understand what's expected. So here I'd recommend going with T or TSubject. However having an ISubject defined would be helpful, if you expect the object to have specific shape. But I also don't get what DoubledObject<T> = T accomplishes :)

duluca · 2017-05-12T14:23:17Z

@sgtoj So here export type DoubledObjectWithKey<T extends string> = { [K in T] }; it is fair, and actually quite helpful to call it TString.

sgtoj · 2017-05-12T14:51:22Z

DoubledObject<T> = T was DoubledObject<Subject> = Subject before I changed it to be consistent with the rest of the definition. I am not sure what the original author's motivation behind the naming of the types defined at the top.

Although I normally see single-letter -- T, K, V, etc -- type parameter names, I am open to the idea of using TObject, etc.

I just looked where the letters originated. Apparently, this convention is adopted from Java and C#.

searls · 2017-05-12T15:01:37Z

I'm not a Typescript user, but I agree that generic types should use single-letter placeholders like T and K,V as those are overwhelmingly common conventions and point out to users that they are customizable.

The real tricky bit that I would want to accomplish from a type awareness for td.js is for any created test double to have its type inferred or specified such that a compilation failure is triggered until the type is created. Same goes with creating a stubbing which then fails compilation and requires the method be written.

To express this with code, what I want is for it to support a workflow like I would have in Java + Mockito. Here's how that would normally flow

public void testInvoices() {
  // 1. Write the fake you want
  LoadsPayments loadsPayments = mock(LoadsPayments.class) 
  // 2. get compiler feedback that you need to go define LoadsPayments (e.g. hit ctrl-1 to quick-fix create the type

  // 3. Write a stubbing of the method you want the fake to define
  Payment payment = new Payment()
  when(loadsPayments.load(new Date())).thenReturn([payment])
  // 4. get compiler feedback that `LoadsPayments` needs an instance method `load` with parameters `(Date)`  and return type `Array<Payment>`, prompting me to go create that in a way that my tools can auto-generate for me

  // … rest of test goes here



}

This sort of paint-by-numbers approach where writing a single isolated test informs the tooling with what it needs to create all the files/classes for you is a fantastic productivity win, and one that encourages lots of small custom types.

How possible is this going to be with TypeScript?

duluca · 2017-05-12T15:03:20Z

Yeah I saw, that was a great change. And I have more C# experience than I care to admit. Given the same guy invented C# and TypeScript, I think it's fair to carry over the conventions. And naming generics start with a capital T (i.e. TSubject or TString) is similar to how you name interfaces with a capital I (i.e. ISubject).

K and V are stand-ins for Key and Value. Personally I prefer TKey and TValue.

duluca · 2017-05-12T15:14:54Z

@searls I know that we can do that easily if we rewrite portions of the app in TypeScript, not sure if it can be achieved through type definitions alone. But then the library can be compiled down to JavaScript and continue to used without any notion of TypeScript by consumers, if they choose to.

Or we could write a wrapper.

searls · 2017-05-12T15:36:35Z

I am entirely okay with this

…

On May 12, 2017, at 11:14 AM, Doguhan Uluca ***@***.***> wrote: @searls I know that we can do that easily if we rewrite portions of the app in TypeScript, not sure if it can be achieved through type definitions alone. But then the library can be compiled down to JavaScript and continue to used without any notion of TypeScript by consumers, if they choose to. ― You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sgtoj · 2017-05-12T17:05:03Z

I am also interested on that project too. May do some work towards it weekend.

duluca · 2017-05-12T22:11:12Z

@sgtoj @searls I've noticed that yarn test:typescript merely transpiles the file, doesn't attempt to run it. Thus not an adequate test harness.
So,

We need to implement test script, which will execute the code
Fix the test code so ln 58: td.replace({}, "prop"); doesn't error out
Only then can this PR be considered as verified by the CI process

@sgtoj let me know if you need help

duluca

We need to implement test script, which will execute the code
Fix the test code so ln 58: td.replace({}, "prop"); doesn't error out

duluca · 2017-05-05T01:49:03Z

index.d.ts

@@ -94,6 +100,16 @@ export function explain<T>(f: TestDouble<T>): Explanation;
 // fake: constructors
 // ----------------------------------------------------------------------------

+/**
+ * Create a fake object constructor the given class.


I think for is missing before the given class

duluca · 2017-05-05T01:51:37Z

index.d.ts

+export function object<T>(object: string): DoubledObject<T>;
+
+/**
+ * Create a fake object that is deep copy of the given object.


a deep copy

sgtoj · 2017-05-26T16:44:24Z

@duluca Sorry... I will review and fix tonight or this weekend. So far, I have already these TS definitions for 2 different work projects; with 2 more in the works. I haven't found any issues yet.

duluca · 2017-05-26T20:53:34Z

In package.json the property "test:typescript" should look like:
"tsc -p ./test/src/typescript && node ./test/src/typescript/test.js && rm ./test/src/typescript/test.js".
This ensure test.js is generated, executed using Node and clean up the file after a successful run. Currently this fails due to the error I had initially reported.

duluca · 2017-06-14T03:03:18Z

@sgtoj I fixed up test.ts in #260, also updated package.json to actually execute the code. So once the PR can be merged, it'd be great to update this PR from master, so there's a valid CI run against this change.

TODO:
[ ] Update from master once #260 is merged
[ ] Update this PR and make sure CI checks pass

searls · 2017-06-18T12:33:13Z

Ping that #260 is merged -- should this be revisited?

duluca · 2017-06-19T13:55:00Z

@sgtoj do you need help with this?

sgtoj · 2017-09-15T17:55:08Z

I have merged from upstream master, fixed one conflict, and fixed one problem pointed out by the test.

duluca

test.ts was intentionally moved under the regression folder

Keeps it out of the src folder to prevent accidental distribution
The CI server compiles and executes the ts code in JavaScript to ensure correct functionality

By moving code from regression to src, you're by-passing CI. Thus making this an invalid PR. Please move test code back in to the regression file.

sgtoj · 2017-09-15T18:47:09Z

That makes sense. I was very confused too. I will fix.

searls · 2017-09-16T00:58:47Z

@duluca please advise if you would like to merge and what version segment is appropriate to bump

duluca · 2017-09-16T14:31:20Z

@searls It makes sense to me as a patch increment.

Brian Ojeda added 4 commits May 4, 2017 10:13

Organize and add func() support

bc20362

Add jsdoc initial pass

c99ef56

Fix minor issues

65ae6ae

Add constructor support

8b9002e

sgtoj mentioned this pull request May 4, 2017

TypeScript: Improve Definition #238

Open

6 tasks

sgtoj changed the title ~~Improved TS Support~~ Improve TS Support May 5, 2017

Brian Ojeda added 3 commits May 10, 2017 21:54

Fix DoubleObjectWithKey type; add respective test

0574e3e

Revert "Fix DoubleObjectWithKey type; add respective test"

3e895af

This reverts commit 0574e3e.

Fix DoubleObjectWithKey type; add respective test; Rename generics

e86769c

duluca suggested changes May 12, 2017

View reviewed changes

Brian Ojeda added 2 commits September 15, 2017 13:36

merge master fix one conflict

f06f6b0

Allow for dynamic args

5ad6b7d

duluca suggested changes Sep 15, 2017

View reviewed changes

Merge typing test file

6c9009f

duluca approved these changes Sep 16, 2017

View reviewed changes

searls merged commit 5a11a57 into testdouble:master Sep 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve TS Support #239

Improve TS Support #239

sgtoj commented May 4, 2017

duluca commented May 5, 2017

duluca commented May 8, 2017

sgtoj commented May 10, 2017

sgtoj commented May 10, 2017

duluca commented May 12, 2017

duluca commented May 12, 2017

sgtoj commented May 12, 2017 •

edited

searls commented May 12, 2017

duluca commented May 12, 2017

duluca commented May 12, 2017 •

edited

searls commented May 12, 2017 via email

sgtoj commented May 12, 2017 •

edited

duluca commented May 12, 2017

duluca left a comment

duluca May 5, 2017

duluca May 5, 2017

sgtoj commented May 26, 2017

duluca commented May 26, 2017 •

edited

duluca commented Jun 14, 2017 •

edited

searls commented Jun 18, 2017

duluca commented Jun 19, 2017

sgtoj commented Sep 15, 2017

duluca left a comment

sgtoj commented Sep 15, 2017

searls commented Sep 16, 2017

duluca commented Sep 16, 2017

Improve TS Support #239

Improve TS Support #239

Conversation

sgtoj commented May 4, 2017

duluca commented May 5, 2017

duluca commented May 8, 2017

sgtoj commented May 10, 2017

sgtoj commented May 10, 2017

duluca commented May 12, 2017

duluca commented May 12, 2017

sgtoj commented May 12, 2017 • edited

searls commented May 12, 2017

duluca commented May 12, 2017

duluca commented May 12, 2017 • edited

searls commented May 12, 2017 via email

sgtoj commented May 12, 2017 • edited

duluca commented May 12, 2017

duluca left a comment

Choose a reason for hiding this comment

duluca May 5, 2017

Choose a reason for hiding this comment

duluca May 5, 2017

Choose a reason for hiding this comment

sgtoj commented May 26, 2017

duluca commented May 26, 2017 • edited

duluca commented Jun 14, 2017 • edited

searls commented Jun 18, 2017

duluca commented Jun 19, 2017

sgtoj commented Sep 15, 2017

duluca left a comment

Choose a reason for hiding this comment

sgtoj commented Sep 15, 2017

searls commented Sep 16, 2017

duluca commented Sep 16, 2017

sgtoj commented May 12, 2017 •

edited

duluca commented May 12, 2017 •

edited

sgtoj commented May 12, 2017 •

edited

duluca commented May 26, 2017 •

edited

duluca commented Jun 14, 2017 •

edited