Skip to content

Commit

Permalink
fix #8.
Browse files Browse the repository at this point in the history
  • Loading branch information
Mathias Lorenzen committed Feb 26, 2019
1 parent 80b8032 commit 0c07eb0
Show file tree
Hide file tree
Showing 7 changed files with 141 additions and 23 deletions.
82 changes: 73 additions & 9 deletions dist/spec/issues/8.test.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/spec/issues/8.test.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/src/Substitute.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ import { ObjectSubstitute, OmitProxyMethods, DisabledSubstituteObject } from "./
export declare const HandlerKey: unique symbol;
export declare const AreProxiesDisabledKey: unique symbol;
export declare class Substitute {
static for<T>(): ObjectSubstitute<OmitProxyMethods<T>, T>;
static for<T>(): ObjectSubstitute<OmitProxyMethods<T>, T> & T;

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

Ha, that is a cute fix.

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

Yup, and it works!

static disableFor<T extends ObjectSubstitute<OmitProxyMethods<any>>>(substitute: T): DisabledSubstituteObject<T>;
}
2 changes: 1 addition & 1 deletion dist/src/Substitute.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 24 additions & 10 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 40 additions & 0 deletions spec/issues/8.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import Substitute from "../../src/Index";
import test from 'ava';

class ClassA {
constructor(){}

methodA(): string {
return 'abc'
}
}

class ClassB {
constructor(
private classA: ClassA
) {}

methodB(): string {
return this.classA.methodA();
}

methodB2(): string {
return 'def'
}
}

class ClassC {
constructor(
private classB: ClassB
) {}

methodC(): string {
return this.classB.methodB2();
}
}

test('issue 9: can record method with 0 arguments', async t => {

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

"issue 8" :-) And also pedantically AFAIU the issue wasn't really 0 arguments, it was that Substitute.for<ClassB> couldn't actually be used as a ClassB due to lacking the private method. Sorry, being pedantic, but saw that #8 was closed and just really wanted to make sure that use case was fixed (as it was a blocker for me last time). This library has the best mocking DSL I've seen for JS/TS so am really looking forward to using it. Thanks!

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

Yeah that typo was fixed in a later commit 😅

The issue is very clear. Without the fix, it won't even compile due to the error you reported. With the fix, it compiles just fine.

Feel free to take your TypeScript Playground example and modify it to include the fix. You'll see that it works.

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

Oh, and thanks for all the praise. Be sure to spread the word if you like it!

Oh, and did you know it can be used with https://github.com/ffMathy/FluffySpoon.JavaScript.Testing.Autofake to make TDD easier, by using an IOC container and automatically faking dependencies?

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

Sure, but I'm just pedantically pointing out for that my use case, whether the constructor had zero arguments or not was irrelevant, it was that the TS mapped type iterators ("for in keyof T") don't iterate over private fields, so:

https://www.typescriptlang.org/play/#src=%0D%0Aexport%20class%20Argument%3CT%3E%20%7B%0D%0A%20%20%20%20encounteredValues%3A%20any%5B%5D%3B%0D%0A%0D%0A%20%20%20%20constructor(%0D%0A%20%20%20%20%20%20%20%20private%20description%3A%20string%2C%0D%0A%20%20%20%20%20%20%20%20private%20matchingFunction%3A%20(arg%3A%20T)%20%3D%3E%20boolean%0D%0A%20%20%20%20)%20%7B%0D%0A%20%20%20%20%20%20%20%20this.encounteredValues%20%3D%20%5B%5D%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20matches(arg%3A%20T)%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20this.matchingFunction(arg)%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20toString()%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20this.description%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20inspect()%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20this.description%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0Aexport%20class%20AllArguments%20extends%20Argument%3Cany%3E%20%7B%0D%0A%20%20%20%20constructor()%20%7B%0D%0A%20%20%20%20%20%20%20%20super('%7Ball%7D'%2C%20()%20%3D%3E%20true)%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0Aexport%20type%20NoArgumentFunctionSubstitute%3CTReturnType%3E%20%3D%20%0D%0A%20%20%20%20(()%20%3D%3E%20(TReturnType%20%26%20NoArgumentMockObjectMixin%3CTReturnType%3E))%0D%0A%0D%0Aexport%20type%20FunctionSubstitute%3CTArguments%20extends%20any%5B%5D%2C%20TReturnType%3E%20%3D%20%0D%0A%20%20%20%20((...args%3A%20TArguments)%20%3D%3E%20(TReturnType%20%26%20MockObjectMixin%3CTArguments%2C%20TReturnType%3E))%20%26%20%0D%0A%20%20%20%20((allArguments%3A%20AllArguments)%20%3D%3E%20(TReturnType%20%26%20MockObjectMixin%3CTArguments%2C%20TReturnType%3E))%0D%0A%0D%0Aexport%20type%20PropertySubstitute%3CTReturnType%3E%20%3D%20(TReturnType%20%26%20Partial%3CNoArgumentMockObjectMixin%3CTReturnType%3E%3E)%3B%0D%0A%0D%0Atype%20BaseMockObjectMixin%3CTReturnType%3E%20%3D%20%7B%0D%0A%20%20%20%20returns%3A%20(...args%3A%20TReturnType%5B%5D)%20%3D%3E%20void%3B%0D%0A%7D%0D%0A%0D%0Atype%20NoArgumentMockObjectMixin%3CTReturnType%3E%20%3D%20BaseMockObjectMixin%3CTReturnType%3E%20%26%20%7B%0D%0A%20%20%20%20mimicks%3A%20(func%3A%20()%20%3D%3E%20TReturnType)%20%3D%3E%20void%3B%0D%0A%7D%0D%0A%0D%0Atype%20MockObjectMixin%3CTArguments%20extends%20any%5B%5D%2C%20TReturnType%3E%20%3D%20BaseMockObjectMixin%3CTReturnType%3E%20%26%20%7B%0D%0A%20%20%20%20mimicks%3A%20(func%3A%20(...args%3A%20TArguments)%20%3D%3E%20TReturnType)%20%3D%3E%20void%3B%0D%0A%7D%0D%0A%0D%0Atype%20ObjectSubstituteTransformation%3CT%20extends%20Object%3E%20%3D%20%7B%0D%0A%20%20%20%20%5BP%20in%20keyof%20T%5D%3A%0D%0A%20%20%20%20T%5BP%5D%20extends%20()%20%3D%3E%20infer%20R%20%3F%20NoArgumentFunctionSubstitute%3CR%3E%20%3A%0D%0A%20%20%20%20T%5BP%5D%20extends%20(...args%3A%20infer%20F)%20%3D%3E%20infer%20R%20%3F%20FunctionSubstitute%3CF%2C%20R%3E%20%3A%0D%0A%20%20%20%20PropertySubstitute%3CT%5BP%5D%3E%3B%0D%0A%7D%0D%0A%0D%0A%0D%0Aclass%20Greeter%20%7B%0D%0A%20%20%20%20private%20name%20%3D%20%22greeter%22%3B%0D%0A%20%20%20%20greet()%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20%22Hello%2C%20%22%20%2B%20this.name%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0Afunction%20greet(g%3A%20Greeter)%20%7B%20return%20g.greet()%20%7D%0D%0Aconst%20g%20%3D%20new%20Greeter()%3B%0D%0Afunction%20mock%3CT%3E()%3A%20ObjectSubstituteTransformation%3CT%3E%20%7B%20return%20undefined%3B%20%7D%0D%0A%2F%2F%20function%20mock%3CT%3E()%3A%20ObjectSubstituteTransformation%3CT%3E%20%26%20T%20%7B%20return%20undefined%3B%20%7D%0D%0A%0D%0Aconst%20mockGreeter%20%3D%20mock%3CGreeter%3E()%3B%0D%0A%0D%0Agreet(mockGreeter)%3B

The & T conveniently "puts the private fields back" AFAIU. Hence my admittedly pedantic assertion that, perhaps myopically focusing on my use case, the simpler/core constraint was around specifically private fields and not constructor arguments (granted, the repro provided in the issue tracker happened to have a private field that was also a constructor argument).

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

(I also did not know about Autofake; thanks for the link :-) )

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

I'm so sorry - I still don't understand what you mean from your example.

It seems like it's working, right?

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

In the playground example, there is a compile error on greet(mockGreeter) without the & T:

Property 'name' is missing in type 'ObjectSubstituteTransformation' but required in type 'Greeter'.

Because name is private, it won't show up in ObjectSubstituteTransformation because the keyof Greeter won't return it.

Which is normally fine/what you want for mapped types, and for interfaces it doesn't matter because they can't have private fields. But for classes, TypeScript's type checking still considers private fields as part of the type, so we need & T to tell TS "sure, pretend this private field is still there".

This "class private fields are considered part of the type" also leads to weird quirks like:

microsoft/TypeScript#6496 (comment)

Where if you have library foo-1.2 and foo-1.3 resolved as separate node modules, even if Greeter in both foo-1.2 and foo-1.3 perfectly structurally match (for public methods), the fact that there is a private field means they're considered different types.

...I think there is a better/different TS issue that describes that nuance vs. the npm link one I linked to, but I can't find it right now.

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

Very nice description, but I am still not sure I see the problem. I see what you are describing - just not how it can be a problem.

Can you modify your playground example to demonstrate the error scenario?

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

Sure, although the playground example is already demonstrating the error scenario, as it has a compile error on line 74, if you hover over it/look for the red squiggly. That's the issue.

FWIW I don't think it ends up being a runtime issue (e.g. I'm not hitting run and having anything blow up), because the JS runtime itself doesn't care whether the private field is there or not, it's solely a TS compiler error.

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

But since the Substitute.for<T>() method will never return the one without the & T type union, how can it become an issue?

That's what I don't understand.

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

"will never return the one without the & T type union" -- now I'm not quite following :-) Do you mean "return" as in at runtime or at compile time?

FWIW at runtime I agree there was no issue prior to this PR or fixed by this PR, because what for returns is a proxy that would magically do whatever T wanted, with or without the & T type union.

It was only at compile time where the TypeScript compiler would step in and, without the & T, say "wait a sec, you can't do const greeter: Greeter = Substitute.for<Greeter>() because that ObjectSubstitute<Greeter> isn't really a Greeter because Greeter has this private field that ObjectSubstitute<Greeter> doesn't have".

But once this PR added & T to the return type, the TS compiler was happy with "oh, okay, you're telling me this really is a greeter with all of its private fields as well, so I'll allow that now".

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

I am so, so sorry. I still don't get it.

Now it sounds like there isn't any issue?

Is it possible for you to make an example of how you would like it to behave?

In your Playground link, you show that a compile error occurs. But after the PR, that should no longer occur. And since you would never actually write out ObjectSubstituteTransformation<T> & T when declaring variables, Substitute.for<T>() would return propagate the type implicitly.

In other words, I still don't see what the issue is, from your example.

This comment has been minimized.

Copy link
@stephenh

stephenh Feb 26, 2019

Now it sounds like there isn't any issue?

Right.

But after the PR, that should no longer occur.

Lol, right, I think we were talking past each other, I was trying to show why before this fix, something with just a private field failed, without the arguments to constructor/etc. in this test case, and totally agree with/after this fix, it works perfectly. Which is great, I was anticipating it being much more annoying. Thanks!

This comment has been minimized.

Copy link
@ffMathy

ffMathy Feb 26, 2019

Owner

Aaaaaah 😂

I see! It's rare that I see people contact my repos to mention that things are working - that's what confused me - hah!

Thank you for your patience with me.

const classBMock = Substitute.for<ClassB>();
const classC = new ClassC(classBMock);
t.not(classC, null);
});
2 changes: 1 addition & 1 deletion src/Substitute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const HandlerKey = Symbol();
export const AreProxiesDisabledKey = Symbol();

export class Substitute {
static for<T>(): ObjectSubstitute<OmitProxyMethods<T>, T> {
static for<T>(): ObjectSubstitute<OmitProxyMethods<T>, T> & T {
const objectContext = new Context();
return objectContext.rootProxy;
}
Expand Down

0 comments on commit 0c07eb0

Please sign in to comment.