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

feat(process): add initial implementations of setImmediate and process.nextTick #10863

Closed
wants to merge 5 commits into from

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Apr 22, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-26574

Description:
This is an implementation of process.nextTick() and global setImmediate/clearImmediate

I tested this usage in our mocha test suite with other node compatibility changes (some of the fs library, modified require())

The two implementations now do work in concert. ticks are drained fully in a while loop. Immediates are done afterwards with some caveats:

  • we give Immediates a 100ms window to process the queue. After that we basically exit the loop and use setTimeout to re-schedule processing the ticks/Immediates remaining.
  • we check if any ticks have been added after an Immediate is run. If so, we immediately drain the tick queue fully again before proceeding in our while loop to process Immediates (and again, the 100ms window limitation may cause us to not process any more immediate right now but do a setTimeout to re-schedule draining the queues)

@sgtcoolguy
Copy link
Contributor Author

Follow-on to #10782

@build build added this to the 8.1.0 milestone Apr 22, 2019
@build
Copy link
Contributor

build commented Apr 22, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3821 tests are passing.
(There are 470 tests skipped)

Generated by 🚫 dangerJS against ef6624f

@@ -147,7 +147,9 @@ process.emitWarning = function (warning, options, code, ctor) { // eslint-disabl
}
this.emit('warning', warning);
};
process.env = {};
process.env = {
// DEBUG: '*' // uncomment to make use of npm 'debug' module. TODO: have CLI pass along env vars in some file/property for dev builds?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be pretty cool to pass environment settings! In our backend, we use https://github.com/motdotla/dotenv for handling environments. And for production builds, things like API keys could be managed by this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah that's a nice solution for production builds. Yeah I was trying to debug some low-level node-api compatibility issues in using mocha on our test suite and hacked in the process.env.DEBUG value here for it and then realized it'd be super-useful to hook our CLI up to generate some files that passed along the ENV for development builds so you could do DEBUG=* ti build -p ios and it just magically worked. I'll probably open a ticket and add that.

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

Just two minor notes on some variable/class naming. The rest looks good!


// Use a nice predictable class/structure for our timers
// JS engine should be able to optimize easier
class Timer {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is a little misleading since it's not really a timer. It's a structure to hold a callback function and its arguments so something like BoundCallback would fit better IMHO.

In Node the object is called Immediate but since we use it for both setImmediate and nextTick that might cause confusion (although it's only used internally for nextTick).

// Use a nice predictable class/structure for our timers
// JS engine should be able to optimize easier
class Timer {
constructor(fun, args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

func is a more clear abbreviation here

Suggested change
constructor(fun, args) {
constructor(func, args) {

@sgtcoolguy sgtcoolguy dismissed janvennemann’s stale review May 15, 2019 19:24

addressed Jan's feedback

@sgtcoolguy
Copy link
Contributor Author

manually merged after rebasing to single commit (and addressing Jan's feedback)

@sgtcoolguy sgtcoolguy closed this May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants