-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add td.instance() #448
Add td.instance() #448
Conversation
Thanks @woldie -- could you take a look at the build and see what's failing? |
Yeah, sorry about that, I thought I had all tests passing. Ok, attempt 2 coming soon. |
@@ -332,6 +332,28 @@ const subject = function (SomeConstructor) { | |||
} | |||
``` | |||
|
|||
#### `td.instance()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please help me refine this documentation section
* @export | ||
* @template T | ||
* @param {{ new (...args: any[]): T }} constructor | ||
* @returns {DoubledObject<typeof T>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not sure what the return type should be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgandecki as resident typescript typings maintainer could you please help with this / add a suggestion for what it should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. Looks correct. I've also verified it by cloning the branch and playing around with it.
Thanks for your work @woldie .
I guess you could possibly get similar results using td.object(Class), but that uses proxy.. and is a bit less safe if used without the additional help of typescript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lgandecki! I agree it would be more convenient to have td.object(Class)
just do it all, but I think it's probably important to cordon off this particular convenience method since it is only a wrapper for a call td.constructor
, but then drops the constructor on the floor when it is done with it, intentionally hiding the ability to mock static class methods.
Looking at this particular change again, I think my use of typeof T
is in the wrong spot for what I want here. I use JSDoc to specify types in my home project and I cribbed from it, but I think I cribbed without checking my work. typeof T
describes a constructor of type T
when used in conjunction with T
by itself which is meant to indicate an instance of T
. When I do a good job with my JSDoc, it works just as well as TypeScript for most things in terms of the quality of IntelliSense and inspections within IntelliJ.
In my home project, I have the need to annotate a constructor, and I needed IntelliJ to understand that the return value from the annotation function was the constructor. Here's how I expressed that in JSDoc:
/**
* @param {typeof T} ctor constructor function.
* @param {...(number|string)} args metadata about ctor.
* @returns {typeof T} annotated constructor function
* @template T
*/
Injector.prototype.annotateConstructor = function (ctor, args) {
...
}
If the JSDoc part of a TypeScript declaration understands typeof T
the same way, then the TypeScript type declaration should read:
/**
* Construct an instance of a faked class.
*
* @export
* @template T
* @param {typeof T} constructor
* @returns {DoubledObject<T>}
*/
export function instance<T>(
constructor: Constructor<T>
): DoubledObject<T>;
I'll try this out later today and see how IntelliJ interprets it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'm unfortunately not really familiar with JSDocs :-(
@@ -0,0 +1,55 @@ | |||
let Thing, SuperThing, fakeInstance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pared down version of constructor.test.js
with assertions I felt were safe to make
|
||
export default function instance (typeOrNames) { | ||
return new (constructor(typeOrNames))() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can redo this if you'd like to recommend a style that better matches the rest of the codebase. Can do the _.tap thing if you think it looks better.
@woldie please also make sure to remove all the console.logs from your branch :) |
Landed in 3.16.0. Thanks @woldie! |
Just pulled 3.16.0 into my project, td.instance works great and many tests got simpler! Thanks @searls and @lgandecki for the assist! |
Attempt to implement #447