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

[TIMOB-13379] Implement console.time/timeEnd #9909

Merged
merged 10 commits into from Jun 8, 2018

Conversation

ewanharris
Copy link
Collaborator

@ewanharris ewanharris commented Mar 6, 2018

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

Implement console.time/timeEnd according to https://console.spec.whatwg.org/#time, with the optional log on console.time('duplicate label') implemented, and a slight deviation to log a warning when calling console.timeEnd('does not exist').

iOS can do a higher precision on the timing due to the implementation being in native but I nerfed for parity, happy to un-nerf though.

Test code

console.time('set label'); // Set custom labels
console.time('set label'); // Should log "Label set label already exists" at a warn lebel
setTimeout(() => {
    console.timeEnd('set label'); // Should log "set label: 100ms" (not exactly 100ms)
}, 100);

console.time(); // Defaults to "default" if no value passed in
setTimeout(() => {
    console.timeEnd(); // Should log "set label: 5000ms" (not exactly 5000ms)
}, 5000);
console.timeEnd('No exist') // Should log "Label No exist does not exist" at warn level

Output

iOS
[WARN]  Label set label already exists
[WARN]  Label No exist does not exist
[INFO]  set label: 106ms
[INFO]  default: 5003ms

Android
[WARN]  Label set label already exists
[WARN]  Label No exist does not exist
[INFO]  set label: 127ms
[INFO]  default: 5004ms


exports.time = function (label) {
if (times[label]) {
exports.warn('Label ' + label + ' already exists');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

imo, this would read nicer using template literals. Which are supported by the v8 version I believe. What're our best practices for the JS code on Android. The eslint options specify ES5

  "parserOptions": {
    "ecmaVersion": 5,
    "sourceType": "script"
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with that since it's supported.

@build
Copy link
Contributor

build commented Mar 6, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

if (!_times) {
_times = [[NSMutableDictionary alloc] init];
}
NSString *label = [args componentsJoinedByString:@""] ?: @"default";
Copy link
Collaborator

@hansemannn hansemannn Mar 6, 2018

Choose a reason for hiding this comment

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

Should a developer be able to pass an array of argumenzts, e.g. console.time(['test', 'foo'])? If not, change the arg-type to id and use ENSURE_SINGLE_ARG(args, NSString); to force a string directly. Example:

- (void)time:(id)label
{
  ENSURE_SINGLE_ARG(label, NSString);
  ...
}

Same for timeEnd probably.

@@ -25,6 +25,7 @@ function join(args) {
}).join(' ');
}

var times = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this to the top of console.js ?

and a warning will be logged to the console.
parameters:
- name: label
summary: The label to track the timer by, defaults to "default".
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there an actual default property you can set here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wasn't aware of that, updated to include default and optional in docs. Making a note to familiarise myself with our doc stuff too :)

if (!label) {
label = 'default';
}
var startTime = times[label];
Copy link
Contributor

Choose a reason for hiding this comment

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

vars should be declared at top of scope, since they get hoisted up there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, given that we know V8 is newer and can support ES6 natively, is there any reason we can't start making use of es6 syntax in runtime JS code? (i.e. template strings here, use of const/let)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sgtcoolguy Updated to use template literals, const and a Map instead of a plain object

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI I filed https://jira.appcelerator.org/browse/TIMOB-25892 to hook the linting up in this folder as grunt lint doesn't include it currently

'ES6-ify' code, add default and optional to documentation
@sgtcoolguy
Copy link
Contributor

Windows PR: appcelerator/titanium_mobile_windows#1209

@sgtcoolguy sgtcoolguy merged commit 323c588 into tidev:master Jun 8, 2018
@jquick-axway jquick-axway modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
@ewanharris ewanharris deleted the TIMOB-13379 branch August 31, 2021 09:44
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

6 participants