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.

Sign up for GitHub

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

Jump to bottom

Add simple rAF polyfill in jsdom environment #4568

Merged
merged 9 commits into from
Sep 30, 2017
Merged

Add simple rAF polyfill in jsdom environment #4568

merged 9 commits into from
Sep 30, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 30, 2017

Summary
Closes #4545

Test plan
Added integration test copied from setImmediate

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017

No idea why it fails on windows. Anyone?

@cpojer
Copy link
Member

cpojer commented Sep 30, 2017

I'm assuming it fails because we don't wait for the setTimeout, which is async, to complete, and the event loop has a different behavior on Windows

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017

Ok, tried a new approach for the test

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017

We should also keep an eye on jsdom/jsdom#1963 so that we can potentially revert this change again in the future

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017 •
edited
Loading

Hmm, new test didn't help. I don't have a windows box available either. Ideas? Skipping it on windows seems like the wrong solution

@cpojer
Copy link
Member

cpojer commented Sep 30, 2017

As said, I suspect the event loop works slightly differently on Windows. Since you are implementing it using setTimeout, you can use jest.useFakeTimers() and then use jest.runAllTimers() after scheduling the rAF call, and it should be called synchronously and make the test fail also on Windows.

If that doesn't work, I'm ok sipping it so we can merge it.

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017

Hmm, after logging out stderr from the test run on appveyor, it seems that raf is not available.

 FAIL __tests__\request_animation_frame.test.js
        × requestAnimationFrame test
      
        ● requestAnimationFrame test
      
          ReferenceError: requestAnimationFrame is not defined
            
            at Object.<anonymous>.done (__tests__/request_animation_frame.test.js:15:3)
                at Promise (<anonymous>)
                at <anonymous>
      
        ● requestAnimationFrame test
      
          expect.hasAssertions()
          
          Expected at least one assertion to be called but received none.
            
            at extractExpectedAssertionsErrors (../../node_modules/expect/build/extract_expected_assertions_errors.js:64:19)
                at <anonymous>
      
      Test Suites: 1 failed, 1 total
      Tests:       1 failed, 1 total
      Snapshots:   0 total
      Time:        1.109s
      Ran all test suites.

Not sure how that happens. I'll see if I'm able to start up a virtual machine and install git, node and yarn to test

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017

I'm confused, the test passes on a Windows 7 image...

image

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2017 •
edited
Loading

Well, upgrading to yarn 1.1.0 made appveyor green 🤷‍♂️

@codecov-io
Copy link

codecov-io commented Sep 30, 2017 •
edited
Loading

Codecov Report

Merging #4568 into master will increase coverage by 0.43%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4568      +/-   ##
=========================================
+ Coverage   55.66%   56.1%   +0.43%     
=========================================
  Files         186     185       -1     
  Lines        6345    6292      -53     
  Branches        3       3              
=========================================
- Hits         3532    3530       -2     
+ Misses       2812    2761      -51     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-environment-jsdom/src/index.js 0% <0%> (ø) ⬆️
.../jest-editor-support/src/parsers/babylon_parser.js 98.57% <0%> (-0.04%) ⬇️
packages/jest-editor-support/src/index.js 0% <0%> (ø) ⬆️
packages/jest-editor-support/src/Snapshot.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfe1a9a...4968028. Read the comment docs.

SimenB
SimenB commented Sep 30, 2017
View reviewed changes
packages/jest-environment-jsdom/src/index.js
const hrInNano = hr[0] * 1e9 + hr[1];
const hrInMicro = hrInNano / 1e6;

return global.setTimeout(callback, 0, hrInMicro);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm currently working on getting it upstreamed into JSDOM, so I read some of the spec. I don't know if people use this argument, or expect it to be in any way accurate, but this is a best effort 🙂

SimenB
SimenB commented Sep 30, 2017
View reviewed changes
packages/jest-environment-jsdom/src/index.js
@@ -22,6 +22,7 @@ class JSDOMEnvironment {
moduleMocker: ?ModuleMocker;

constructor(config: ProjectConfig): void {
const jsdomInitialized = process.hrtime();
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to have it beneath the JSDom.jsdom call, but that makes stuff fail with an odd flow error...

@cpojer cpojer merged commit 216e8ed into jestjs:master Sep 30, 2017
@SimenB SimenB deleted the raf branch September 30, 2017 20:49
@kwelch kwelch mentioned this pull request Oct 1, 2017
Merged
tabrindle pushed a commit to tabrindle/jest that referenced this pull request Oct 2, 2017
@SimenB @tabrindle
Add simple rAF polyfill in jsdom environment (jestjs#4568)
1c6119c 
* Add simple rAF polyfill in jsdom environment

Closes jestjs#4545

* Fix flow error

* Tweak test

* Try to log out stderr on CI

* Use snake case naming for test file

* Update to newest yarn on ci

* Revert "Try to log out stderr on CI"

This reverts commit 08d58c5.

* Remove extra -- from appveyor to avoid warning on newer yarn

* Include time since window initialised in rAF implementation
@taion taion mentioned this pull request Oct 2, 2017
Closed
@hjylewis
Copy link

When will this be released?

@SimenB
Copy link
Member Author

SimenB commented Oct 13, 2017

It is out in 21.3.0-beta.2

@gaearon gaearon mentioned this pull request Oct 28, 2017
Closed
@mAiNiNfEcTiOn mAiNiNfEcTiOn mentioned this pull request Nov 15, 2017
Merged
@renovate-tests renovate-tests mentioned this pull request Feb 6, 2018
Closed
@renovate renovate bot mentioned this pull request Feb 14, 2018
Open
@Robdel12 Robdel12 mentioned this pull request Oct 31, 2018
Merged
@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 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed ✔️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

requestAnimationFrame warning with React 16
5 participants
@SimenB @cpojer @codecov-io @hjylewis @facebook-github-bot