-
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
Implement new preprocessData and types to support new React marks #20
Conversation
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.
I didn't check the tests since I'm not very familiar with testing so I'll leave that to @jevakallio , everything else looks good except with some changes left to make.
expect( | ||
// prettier-ignore | ||
preprocessData([ | ||
{"args":{"data":{"documentLoaderURL":"https://concurrent-demo.now.sh/","isLoadingMainFrame":true,"navigationId":"43BC238A4FB7548146D3CD739C9C9434"},"frame":"FD65D9AFD04B1295CEA36B883F0FA82F"},"cat":"blink.user_timing","name":"navigationStart","ph":"R","pid":9312,"tid":10252,"ts":8993749139,"tts":1646191}, |
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 array is extremely long and it may be a good idea to put it in a separate JSON file, but I think there's some value in having the data visible when you read the tests, instead of having to poke in another file. Opinions welcome but I think this inlined array is good enough for now.
Merging to unblock #24. @jevakallio feel free to comment further if you have anything :) |
Summary
Implements
preprocessDataV2
and add other V2 types, to support the new user timing marks added in MLH-Fellowship/react#11.preprocessDataV2
is not being used in app code yet, but tests have been written to ensure that the implementation is mostly correct.Also added new profiles exported from the Chrome performance tab + versions filtered to contain only
blink.user_timing
events.Resolves #13.
Design
preprocessDataV2
is largely adapted from Brian's originalpreprocessData
. The biggest design change I made was to split the React codebase-style pseudo-class/method structure into completely separate functions. Imo this makes it much easier to trace the code as the inputs and outputs of each function is clear, instead of allowing all "methods" in the pseudo-class to mutate some shared variables.ReactEventV2
is now a disjoint union type. Not sure how useful this will be, but for now this allows us to enforce the presence of some fields that only apply to certain React events. I did not changeReactMeasureV2
as they all seem to contain the same fields.Would love to know your thoughts.
Rollout plan
Test plan
yarn test