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: compute and log build time #5602

Merged
merged 2 commits into from
Dec 8, 2021

Conversation

amaaniqbal
Copy link
Contributor

PR Checklist

What is the current behavior?

Build time not shown in ios.

What is the new behavior?

Build time is computed and logged irrespective of OS.

Fixes/Implements/Closes #2998.

@project-bot project-bot bot added this to Pull Request in CLI Team Oct 27, 2021
@cla-bot cla-bot bot added the cla: yes label Oct 27, 2021
@amaaniqbal
Copy link
Contributor Author

@rigor789 Please have a look at this PR. Currently, I am logging the build time in seconds. Let me know if I should log the build time in ms or some other format.

@amaaniqbal amaaniqbal changed the title Compute and log build time feat: compute and log build time Oct 27, 2021
@rigor789
Copy link
Member

rigor789 commented Oct 27, 2021

Hey @amaaniqbal - seconds is fine, think most tools log in seconds (including fractions) - this looks fine, just want to run this on both ios/android to see it in action, code LGTM 👍

@amaaniqbal
Copy link
Contributor Author

seconds is fine, think most tools log in seconds (including fractions)

This was my intuition too 😄.

this looks fine, just want to run this on both ios/android to see it in action

Sure, let me know how it works on your end.

@rigor789 rigor789 added this to the 8.2 milestone Nov 23, 2021
@rigor789 rigor789 merged commit fb0a7b8 into NativeScript:master Dec 8, 2021
CLI Team automation moved this from Pull Request to Done Dec 8, 2021
@edusperoni
Copy link
Collaborator

edusperoni commented Dec 9, 2021

this throws performance is not defined on our CI build

conext:

  # android list targets

  sudo apt-get update
  sudo apt-get install -y libhybris-utils

  # setting up correct versions of node/npm
  nvm install 14
  nvm use 14
  nvm alias default current

  # update npm to latest
  npm i -g npm semver colors
  npm config set spin false
  npm config set progress false

  # update yarn
  npm i -g yarn

  gem update bundler -N

  echo -e "n\n" | npm i -g nativescript@next
  ns usage-reporting disable
  ns error-reporting disable

@rigor789
Copy link
Member

rigor789 commented Dec 9, 2021

@edusperoni good to know, we can use a different time source I guess, or a polyfill.

It is strange however, because according to the node docs, performance.now has been available since node 8.5

https://nodejs.org/api/perf_hooks.html#performancenow

Do you have more context around the error? Can you try running the CI build with --log trace?

Edit: I think we just need to import { performance } from "perf_hooks";

Added it in fade332 - you can test once the CI releases it.

@edusperoni
Copy link
Collaborator

Yes, that fix solves it. I was just testing it locally and import { performance } from "perf_hooks"; is required for it to work on node.

@rigor789
Copy link
Member

rigor789 commented Dec 9, 2021

@edusperoni I tested the above and worked on my end without the import, using node v15.11.0. 😕

@edusperoni
Copy link
Collaborator

@rigor789 I'm using node 14, maybe they changed it in 15?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
CLI Team
  
Done
Development

Successfully merging this pull request may close these issues.

Doesn't show time taken to build for ios
3 participants