Make defaultResolver type more flexible #10
Conversation
|
Sounds good to me! Should I bump it as a major, or is minor fine? |
|
There should be no change to current behavior, since it's just making the types a bit more correct as to the existing behavior (while opening the way for future changes to jest). A patch would probably be fine, but it can be minor if you'd like as well. |
|
Hi @arcanis, do you have any thoughts on this change? Thanks! |
|
Hi, just another friendly ping on this small PR that it sounded like you were on board with. |
|
👋 there! Any chance you could merge this? |
|
I'm cleaning up my open PRs that don't seem to have a chance of merging. |
|
Hey @IanVS, apologies, I completely missed your pings - I'll look at applying and publishing your patch later today (I assume you still need it?) |
|
I actually don't know if it's still needed. I haven't been very involved in jest development since this. According to my comment jestjs/jest#11540 (comment), it sounds like this is a good change to make regardless. But @SimenB would be more knowledgable about it at this point I think. |
|
Hah wow, old one 😅 is there anything to do here? |
|
Async resolver landed some time ago, fwiw |
Yep, I remember you worked quite closely with me on that, thanks. I think this change is only needed if you wanted to re-add a default async resolver, which was removed here: jestjs/jest@ Currently, it seems that |
|
Right, but does that matter? Async can call into sync without issue. Is there any feature (type or otherwise) that needs this? (Thanks again for the patience in that PR 😀) |
|
I think so, yes, because of the difference in the shape of It's all starting to come back to me now… So yeah, I think this is still a good change to make. I'll reopen.
Haha, likewise! |
| request: string, | ||
| options: JestResolverOptions, | ||
| ): string; | ||
| export default function resolve<D extends (request: string, options: JestResolverOptions<D>) => any>( |
There was a problem hiding this comment.
Is any a good default? Should it be a string?
There was a problem hiding this comment.
That was the original rub, that the defaultResolverAsync returned a promise and not a string. Here's a playground I threw together, and I'm not sure how to specify anything other than any here. (I tried unknown and string | Promise<string>)
There was a problem hiding this comment.
A resolver should always return a string or null (from memory, I can very tomorrow)
There was a problem hiding this comment.
Even an async resolver? Wouldn't it return a promise?
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like that would work, yeah. Here's a version that's a bit more strict and expects string or null (or promise of one) to be returned from the resolver https://tsplay.dev/mA2Z8W
Though in the end, I wonder if this should really care what the resolver returns. So maybe your version is better.
There is work ongoing in jest to allow async resolvers, and since jest-pnp-resolver is built-in, we'll need a small tweak to the typescript types. This PR just makes the types a bit more flexible, to allow an async
defaultResolverwhile continuing to also support sync as well.The trick here is using the
ReturnTypeof the default resolver as the return type of the pnp resolver.