Skip to content

feat(core): queueMacroTask - #8904

Merged
NathanWalker merged 6 commits into
NativeScript:masterfrom
edusperoni:macrotasks
Nov 12, 2020
Merged

NathanWalker merged 6 commits into
NativeScript:masterfrom
edusperoni:macrotasks

Conversation

@edusperoni

Copy link
Copy Markdown
Contributor

PR Checklist

What is the current behavior?

The only way to schedule a macrotask is by using setTimeout, and even that isn't a guaranteed to be executed immediately (ready: before frame) on iOS.

Here's an example (for some reason playground is not saving the new version, so comment the last setTimeout of home.component.ts` or you'll freeze for a few seconds at the home page): https://play.nativescript.org/?template=play-ng&id=B4UY5b&v=2

What is the new behavior?

Defines a proper scheduler for macroTasks by queuing a task on the main thread) that will run after all microTasks have ran. requestAnimationFrame is now in a macroTask to mimic web behavior (this is necessary because angular will use requestAnimationFrame to schedule it's change detection).

Looking for input in error handling and if we should schedule multiple tasks instead of scheduling one and iterating through the callbacks.

@cla-bot cla-bot Bot added the cla: yes label Sep 25, 2020
@rigor789
rigor789 marked this pull request as draft September 25, 2020 21:43
@NathanWalker NathanWalker added this to the 8.0 milestone Sep 28, 2020
@NathanWalker
NathanWalker marked this pull request as ready for review September 28, 2020 21:17
@NathanWalker
NathanWalker marked this pull request as draft September 28, 2020 21:23
@edusperoni

edusperoni commented Sep 28, 2020 •

Copy link
Copy Markdown
Contributor Author

Updated to latest master. As soon as we settle on error handling this is good to go.

Might be worth mentioning that setTimeout(fn,0) will run after queueMacrotask on iOS as it uses timers. On android setTimeout(fn,0) has the same priority as queueMacrotask (whichever gets scheduled first runs first).

@NathanWalker

Copy link
Copy Markdown
Contributor

All tests passing thanks, we can do some testing against frontend flavors - definitely want to check against Vue, React and Svelte.

@NathanWalker NathanWalker modified the milestones: 8.0, 7.1 Oct 5, 2020
@edusperoni edusperoni changed the title DO NOT MERGE: feat(core): queueMacroTask feat(core): queueMacroTask Oct 8, 2020
@NathanWalker NathanWalker added the ready for test TSC needs to test this and confirm against live production apps and automated test suites label Oct 27, 2020
@NathanWalker
NathanWalker marked this pull request as ready for review October 27, 2020 21:38
@NathanWalker
NathanWalker merged commit e3dc89f into NativeScript:master Nov 12, 2020
@CatchABus

Copy link
Copy Markdown
Contributor

@edusperoni @NathanWalker I know this PR was approved, but it causes 2 errors.
Inside animation-frame and macro-task modules, a bad import of trace module is used.
Should change from this:
import { Trace } from 'trace';
to this:
import { Trace } from '../trace';

@farfromrefug

Copy link
Copy Markdown
Collaborator

@dimitrisrk @NathanWalker created a PR #9027.
I think it should be a good thing for you to always run the automated app when approving a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes ready for test TSC needs to test this and confirm against live production apps and automated test suites

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants