-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Optimize flamechart][1/4] Add flamechart to ReactProfilerData #90
Conversation
This commit begins a stack of PRs that optimizes our flamechart. Broadly, we need to replace the use of Speedscope's flamechart types with our own types that are optimized for our rendering code. As flamechart was always moved around together with ReactProfilerData, it makes sense to move flamechart into it. That also necessitates moving preprocessFlamechart into preprocessData. This PR works towards #50. * `yarn flow`: errors present, but no errors in affected code. * `yarn lint` * `yarn start`
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mlh-fellowship/scheduling-profiler-prototype/day54m8b4 |
@@ -81,7 +70,8 @@ export default function ImportPage({onDataImported}: Props) { | |||
</a>{' '} | |||
from Chrome Devtools. | |||
<br /> | |||
To zoom, scroll while holding down <kbd>Ctrl</kbd> or <kbd>Shift</kbd> | |||
To zoom, scroll while holding down <kbd>Ctrl</kbd> or{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from #89's commit, that's also in this PR.
@kartikcho I'll be trying to rebase merge this PR, to see if #89's commit will appear twice on master (let's hope it doesn't). If #89's commit appears twice, I'll force push to the remaining PRs in this stack before merging them, to prevent polluting master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Only one commit appeared on master :D We're all good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Wait is that what LGTM means or is it Let's Get This Merged?
export default function preprocessData( | ||
timeline: TimelineEvent[], | ||
): ReactProfilerData { | ||
const profilerData = { | ||
const flamechart = preprocessFlamechart(timeline); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Might wanna remove this extra line for no reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This empty line was intentional as I want to separate the profiler data declaration that belongs to the rest of this function from the flamechart computation. Imo they're doing different things conceptually so they should be in separate "paragraphs"
Stack PR by STACK ATTACK:
Summary
This commit begins a stack of PRs that optimizes our flamechart.
Broadly, we need to replace the use of Speedscope's flamechart types
with our own types that are optimized for our rendering code.
As flamechart was always moved around together with ReactProfilerData,
it makes sense to move flamechart into it. That also necessitates moving
preprocessFlamechart into preprocessData.
This PR works towards #50.
Test Plan
yarn flow
: errors present, but no errors in affected code.yarn lint
yarn start