[mdn] Initial experiment for adding performance tool #33045
Conversation
| console.log(result); | ||
| }, 300000); | ||
|
|
||
| it('should measure a component with heavy rendering load', async () => { |
There was a problem hiding this comment.
I don't think this test adds much value outside of slowing everything down. It might be worth having tests for the unhappy paths, for example where there's invalid syntax, imports to some unknown/non-present module, etc
Great job adding tests btw!
|
|
||
| expect(result).toHaveProperty('renderTime'); | ||
| expect(result).toHaveProperty('webVitals'); | ||
| console.log(result); |
There was a problem hiding this comment.
nit: these console.logs can be removed before merging, they tend to clutter output
| // Launch puppeteer with increased protocol timeout | ||
| const browser = await puppeteer.launch({ | ||
| protocolTimeout: 600_000, // Increase timeout to 10 minutes | ||
| headless: false |
There was a problem hiding this comment.
Probably fine for debugging, but seems like this would be less irritating for the user if it was headless?
| * @param {string} code - The source code to parse | ||
| * @returns {babel.types.File} - The parsed AST | ||
| */ | ||
| function parseAsync(code: string) { |
There was a problem hiding this comment.
This is already exported from babel: https://babeljs.io/docs/babel-core#parseasync. You'll also probably want to use @babel/preset-react and also tune the settings so its maximally compatible with all types of syntax since generally since we have no control over what the user is going to paste in. Might also be good to return a helpful error message if it couldn't parse / transform, since it can help the LLM adjust the syntax and try again.
One more thing you'll want to add is
configFile: false,
babelrc: false,as otherwise Babel will try to look for a babel.config.js somewhere and it can throw things off.
These config options can also be shared with transformFromAstAsync I believe.
| React.createElement(React.Profiler, { | ||
| id: 'App', | ||
| onRender: (id, phase, actualDuration) => { | ||
| window.__RESULT__.renderTime = actualDuration; |
There was a problem hiding this comment.
could probably capture all the arguments
| webVitals.onTTFB((metric) => { window.__RESULT__.webVitals.ttfb = metric; }); | ||
|
|
||
| // Wrap user code in React.Profiler to measure render performance | ||
| const renderStart = performance.now(); |
There was a problem hiding this comment.
also this seems unnecessary given that React.Profiler already gives you the time taken to mount.
| - First Input Delay (FID): ${performanceResults.webVitals.fid?.value || 'N/A'}ms | ||
| - Time to First Byte (TTFB): ${performanceResults.webVitals.ttfb?.value || 'N/A'}ms | ||
|
|
||
| These metrics can help you evaluate the performance of your React component. Lower values generally indicate better performance. |
There was a problem hiding this comment.
This is a nice touch. Could you try asking it to run this tool multiple times to see if it improves on these metrics?
One more thing we might want to do is to run several iterations of the test in a single tool call to reduce noise. Just using the mean for now seems like a good start.
| expect(result.error).toBeNull(); | ||
| }, 300000); | ||
|
|
||
| it('should fail due to using imported useState', async () => { |
There was a problem hiding this comment.
Right now only because the way I'm transpiling can't deal with imports and expects everything to use React. prefix. I want to find a way around this but maybe as a forward fix because I have multiple ideas on how to do that
There was a problem hiding this comment.
In that case then this is not a good test since it's only a temporary quirk and not an invariant that is necessary for correctness. I would just remove this test for now since you would have to delete it later anyway.
|
|
||
| await page.waitForFunction( | ||
| 'window.__RESULT__ !== undefined && (window.__RESULT__.renderTime !== null || window.__RESULT__.error !== null)', | ||
| {timeout: 600_000}, |
There was a problem hiding this comment.
small note but i don't think this timeout needs to be that long. If it takes over 10 seconds something is probably wrong. Feel free to address this in a follow up PR though.
Summary
Add a way for the agent to get some data on the performance of react code
How did you test this change?
Unit tests