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(android): TIMOB-26541 setTimeout and setInterval do not support calls without interval specified #10451

Merged
merged 2 commits into from Nov 26, 2018

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Nov 9, 2018

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

Description:

  • Update timers test suite
  • fix Android implementation to support unspecified interval for timer

The update unit test suite really covers the use cases much better now, and specifically tests for this bug. Also note that the iOS implementation was fixed in #10426 (https://github.com/appcelerator/titanium_mobile/pull/10426/files#diff-4c4d01060a96380d965a014b8b0c3186R116)

…alls without interval specified

* Update timers test suite
* fix Android implementation to support unspecified interval for timer

Fixes TIMOB-26541
@build
Copy link
Contributor

build commented Nov 9, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@lokeshchdhry
Copy link
Contributor

FR Passed.

Studio Ver: 5.1.1.201809051655
SDK Ver: 8.0.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.1
Appc NPM: 4.2.13
Appc CLI: 7.0.8-5
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.4
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Nexus 6P (Android 8.1.0)

@lokeshchdhry
Copy link
Contributor

Waiting for CR to merge.

@jquick-axway
Copy link
Contributor

Regarding setInterval(), according to Mozilla and Node.js, the delay parameter is required.
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setInterval
https://nodejs.org/api/timers.html#timers_setinterval_callback_delay_args

However, I tested the below in Node.js and it works. If the delay parameter is not provided, then it defaults to 1ms. At least on MacOS (I would expect the min to be 10-16ms on Windows). This appears to be an undocumented feature in Node.js.

setInterval(function() {
	console.log("@@@ UnixTime: " + (new Date()).getTime());
});

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@sgtcoolguy sgtcoolguy merged commit 621d6c4 into tidev:master Nov 26, 2018
@sgtcoolguy sgtcoolguy deleted the android-setTimeout branch November 26, 2018 16:34
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