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

ReactPerf 15.1.0-alpha.1 expose isProfiling on the exported object? #6762

Closed
nfcampos opened this issue May 12, 2016 · 6 comments · Fixed by #6763
Closed

ReactPerf 15.1.0-alpha.1 expose isProfiling on the exported object? #6762

nfcampos opened this issue May 12, 2016 · 6 comments · Fixed by #6763

Comments

@nfcampos
Copy link

I've tested the new Perf tools on one of the screens in one app I'm working on and it works great :)

One thing I wanted also on the old one Perf tools was to be able to check whether the Perf tools are started or stopped, so basically exposing ReactDebugTool's isProfiling variable. My use for that is that I have a keyboard shortcut (in development) bound to start/stop the Perf tools and currently I'm forced to maintain such a variable myself to know whether to start or stop (which gets out of sync if I start/stop the perf tools without the keyboard shortcut).

If you don't think it's worthwhile to expose isProfiling feel free to close this issue.

@gaearon
Copy link
Collaborator

gaearon commented May 12, 2016

ReactDebugTool is not a public API by itself, at least not yet. I’m open to adding isRunning() to ReactPerf though, as it’s the public API. Would you like to send a PR?

@nfcampos
Copy link
Author

Yeah I'll send a PR :) Should this isRunning() method on ReactPerf check ReactDebugTool's isProfiling, which is not currently exposed, or should keep track of that state separately?

@gaearon
Copy link
Collaborator

gaearon commented May 12, 2016

Yeah, we can add isRunning() to ReactPerf and isProfiling() to ReactDebugTool with the real state being in ReactDebugTool.

@nfcampos
Copy link
Author

the PR should be against which branch? master?

@gaearon
Copy link
Collaborator

gaearon commented May 12, 2016

Yep!

@nfcampos
Copy link
Author

I've opened the PR #6763

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

Successfully merging a pull request may close this issue.

2 participants