Skip to content
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

fix(env-jsdom): remove setImmediate and clearImmediate #11222

Merged
merged 7 commits into from
Mar 25, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Mar 20, 2021

Summary

Fixes #11204

Test plan

Green CI

@SimenB
Copy link
Member Author

SimenB commented Mar 20, 2021

Bah, this fails since source-map-support requires Buffer... Gonna be somewhat painful this 😛

@SimenB SimenB force-pushed the remove-node-globals-from-jsdom branch from 7bde71e to aea4188 Compare March 20, 2021 09:00
@SimenB SimenB force-pushed the remove-node-globals-from-jsdom branch from aea4188 to 1e15a27 Compare March 20, 2021 09:02
@@ -1823,6 +1821,11 @@ export default class Runtime {
throw moduleNotFoundError;
}

e.message = `Jest: Got error evaluating ${path.relative(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was impossible to see where errors originated without this

Before: image

After: image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not have that now though to reduce the surface of this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow I ran into that reference error yesterday for some reasons and it was gone out of the blue

@SimenB
Copy link
Member Author

SimenB commented Mar 20, 2021

This breaks @testing-library/dom: testing-library/dom-testing-library#914

)
.install(sourcemapOptions);
if (isBrowserEnv) {
runtime.requireInternalModule(
Copy link
Member Author

@SimenB SimenB Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason this is horribly slow:

image

If I comment out the install call (but keeping the require):

image

Note that the install itself is fast, it's whatever happens when something throws later that that takes 10+ seconds...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LinusU any ideas about what's happening here? I'm having some issues reproducing this using plain JSDOM, so I'm not really sure where to begin debugging this...

@SimenB
Copy link
Member Author

SimenB commented Mar 22, 2021

Might land removal of {set,clear}Immediate and leave Buffer in for now if the source map stuff proves elusive. Regardless we need to wait for (or patch locally, which is meh) for @testing-library/dom to handle the timer functions missing, so no need to decide right now

@gaearon
Copy link
Contributor

gaearon commented Mar 25, 2021

Does testing-library/dom-testing-library#916 unblock you?

@SimenB
Copy link
Member Author

SimenB commented Mar 25, 2021

Yep, it does! 👍

If it's merged and released I'll just keep the Buffer change separate until we figure out why it makes that test so horribly slow.

Alternatively I can just use yarn patch, but hopefully that PR lands soonish so we don't need such workarounds

@renatoalencar
Copy link

Hi @SimenB, I added the following test case in order to reproduce the issue. Let me know if that covers it.

  test('safe check for setImmediate and clearImmediate', () => {
    const setImeediate = global.setImmediate
    const clearImmediate = global.clearImmediate
    delete global.setImmediate
    delete global.clearImmediate

    expect(() => runWithRealTimers(() => {})).not.toThrow()

    global.setImmediate = setImeediate
    global.clearImmediate = clearImmediate
  })

@renatoalencar
Copy link

Hi @SimenB, I added the following test case in order to reproduce the issue. Let me know if that covers it.

  test('safe check for setImmediate and clearImmediate', () => {
    const setImeediate = global.setImmediate
    const clearImmediate = global.clearImmediate
    delete global.setImmediate
    delete global.clearImmediate

    expect(() => runWithRealTimers(() => {})).not.toThrow()

    global.setImmediate = setImeediate
    global.clearImmediate = clearImmediate
  })

From the tests I've made it throws the same exception.

Screenshot from 2021-03-25 13-24-42

@gaearon
Copy link
Contributor

gaearon commented Mar 25, 2021

@renatoalencar I don't understand what line is pictured on your screenshot. It shouldn't exist after your change, should it? setImmediate is no longer listed in that object unconditionally.

@renatoalencar
Copy link

renatoalencar commented Mar 25, 2021

@renatoalencar I don't understand what line is pictured on your screenshot. It shouldn't exist after your change, should it? setImmediate is no longer listed in that object unconditionally.

Yeah, I just putted it there so I could reproduce the error through the tests. I wanted to see if that would happen by just deleting setImmediate and clearTimeout from the global object and calling runWithRealTimers.

I wrote the tests after the code, so I'm just showing that they really cover this case.

@gaearon
Copy link
Contributor

gaearon commented Mar 25, 2021

Ahh got it.

@SimenB SimenB changed the title fix(env-jsdom): remove node-specific globals fix(env-jsdom): remove setImmediate and clearImmediate Mar 25, 2021
@SimenB SimenB merged commit 167aad4 into jestjs:master Mar 25, 2021
@SimenB SimenB deleted the remove-node-globals-from-jsdom branch March 25, 2021 18:50
@SimenB
Copy link
Member Author

SimenB commented Mar 25, 2021

Following up on Buffer here: #11241

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Sandbox setImmediate() in jsdom environment
5 participants