-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
chore: migrate jest-environment-jsdom to TypeScript #8003
Conversation
This reverts commit bd4e070.
👀 I'll hold off on commenting just yet then 😀 |
Yes, please, the code is FULL or errors. At this point I just need some guidance. |
Q1: |
Q2: To be able to reset this.global, we need to change it's type to accept null. This is currently not compatible with Q2b: Also, should we also set this.global to null on teardown in |
Yeah, we have a TODO it it should accept
Yeah, feel free to make it nullable
Yeah, I think that makes sense - easier for GC to pick it up then |
This reverts commit 3c4486f.
Do you prefer if this goes into a separate PR, since it's another package? Or doesn't matter? |
Doesn't matter |
@SimenB please take a look when you have the time, and let me know what you think. There are quite a few discrepancies between types that technically should match. I have solved these with I've changed the return type of |
if (this.dom) { | ||
// Explicitly returning `unknown` since `runVMScript` currently returns | ||
// `void`, which is wrong | ||
return this.dom.runVMScript(script) as unknown; |
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.
return this.dom.runVMScript(script) as unknown; | |
return this.dom.runVMScript(script) as any; |
We know what the type here will be, no need to assert it.
(it'll have the EVAL_RESULT_VARIABLE
object thing due to https://github.com/facebook/jest/blob/438a178c8addf2806bebf44726d080921ad9984f/packages/jest-transform/src/ScriptTransformer.ts#L559-L565)
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.
We do need to assert it since runVMScript()
returns void
, which is not true, and runScript()
is also expected to receive {[ScriptTransformer.EVAL_RESULT_VARIABLE]: ModuleWrapper} | null
(via the JestEnvironment interface). I have a new solution for this, committing in a sec.
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.
We know what the type here will be
This contrasts with #8003 (comment) - we can't both set the return type to the EVAL_RESULT_VARIABLE
thing, AND the return value of the provided script... Sorry, I'm a bit confused here.
If we want the return type of runScript
to be the the same as the return type of the provided script, we could use a generic to let the user of jest-environment-jsdom
set the return type? Just an idea. But I feel there is something I am missing 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.
The return value of the provided script is, 100%, the EVAL_RESULT_VARIABLE
thing (unless the environment has been torn down, in which cse it will be null
). We construct the script ourselves. The function assigned to EVAL_RESULT_VARIABLE
, we do not know what will return when invoked (that's the unknown
part), but we don't care about that result
If we want the return type of
runScript
to be the the same as the return type of the provided script, we could use a generic to let the user ofjest-environment-jsdom
set the return type? Just an idea. But I feel there is something I am missing here... :)
I don't think that's needed - the script we get is constructed from a string source we've read (and potentially transformed) from disk. So we cannot statically know what it will return
@lirbank I pushed a commit that removes some of the comments to reduce the diff - this allows git to see it's a file rename and retain blame history |
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.
This was a tough one! I think it's good to go now, though 🙂 I added a diff to the OP which looks good. The generated typings file also looks good. Will merge if CI is happy.
Thank you so much for sticking with me through this review!
Also, I've opened up a PR to definitelytyped to fix the JSDOM type: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33476/files CI is passing! :🎉@lirbank could you mark this PR ready for review? I'm not allowed to merge when it's a draft 🙂 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
@SimenB I have a bunch of questions. I'll make a few at the time to see if it clears to me as we go (see inline review comments in a sec).
Built diff: