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

Deferred: Deprecate setTimeout() usage under latest browsers. #5114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

diegocr
Copy link

@diegocr diegocr commented Sep 10, 2022

Summary

Hi there, as you may know, Chrome/ium has been lately aggressively throttling background tabs, this may result in those setTimeout()-based promises/deferreds to never resolve until the tab becomes active again.

This PR is an attempt to prevent that, by using queueMicrotask under browsers supporting it. Also, the logic has been changed to dispatch those processes in bulk instead of individually, since the main reason for that throttling seems to be by having too many concurrent setTimeout()s running.

This implements a new jQuery.asap() function that will use either queueMicrotask or setTimeout for older browsers.

I've observed that around this whole codebase there are several other uses of that window.setTimeout( func ) pattern so perhaps I should move this jQuery.asap() elsewhere to be used in place of those as well (as part of another PR) ?

Thanks!

Checklist

  • New tests have been added to show the fix or feature works
  • Grunt build and unit tests pass locally with these changes
  • If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 10, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: diegocr / name: Diego Casorran (813b77c)

@mgol mgol added Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Sep 10, 2022
src/deferred.js Outdated Show resolved Hide resolved
@mgol
Copy link
Member

mgol commented Sep 12, 2022

Thanks for the PR. Please fix the reported ESLint violations. Also, we don't want to expose a new method, please keep it internal. Once that's done, we should see the actual test suite running.

Also, do we need a manual queue when using queueMicrotask? Won't the browser manage such a queue by itself?

@mgol mgol added Needs info Deferred and removed Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Sep 12, 2022
@diegocr
Copy link
Author

diegocr commented Sep 12, 2022

Hi, thank you, pleased you guys are positive about those kind of changes.

Well, the manual queue for queueMicrotask can be seen as subjective yes, i think it could help the browser but probably we won't prevent much overhead there. Also, i was expecting it'd be fine exposing a method so that for example i could then replace it with my own scheduler. If these are no-go then we can simplify this a lot, will now proceed to do so.

@diegocr diegocr changed the title Deferred: Deprecate setTimeout() usage, and dispatch processes in bulk. Deferred: Deprecate setTimeout() usage under latest browsers. Sep 12, 2022
@diegocr diegocr force-pushed the patch-1 branch 2 times, most recently from 6d523f3 to fd13506 Compare September 14, 2022 13:39
@mgol
Copy link
Member

mgol commented Sep 20, 2022

@diegocr you need to rebase the PR to avoid the issues on main as they have recently been fixed.

@diegocr
Copy link
Author

diegocr commented Sep 20, 2022

Cool, done 🚀

@mgol mgol added this to the 4.0.0 milestone Oct 3, 2022
@mgol mgol self-assigned this Oct 3, 2022
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Sorry for a late review. I have one doubt about the testing strategy.

Comment on lines +1531 to +1536
this.sandbox.stub( jQuery, "readyException" ).callsFake( function( error ) {
assert.strictEqual( error.message, expected, "jQuery.readyException called with unexpected error" );
window.setTimeout( function() {
throw error;
} );
} );
Copy link
Member

Choose a reason for hiding this comment

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

Why mocking jQuery.readyException? Can't this test be done in a similar way as the setTimeout one? In that case, maybe we could just keep a single test?

@mgol mgol removed the Needs review label Feb 13, 2023
@timmywil timmywil modified the milestones: 4.0.0, 4.1.0 Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants