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.

Sign up for GitHub

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

Minor adjustments to profiler #11376

Merged
merged 15 commits into from
Nov 21, 2023
Merged

Minor adjustments to profiler #11376

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bf8c304
Use shorthand return in peekRender
jerelmiller Nov 20, 2023
d1976a9
Use the iteratorPosition in getCurrentRender as the current render
jerelmiller Nov 20, 2023
53333ca
Rename currentRenderCount to totalRenderCount to clarify intent & upd…
jerelmiller Nov 20, 2023
3a48ead
Remove takeUntilRenderCount function for now
jerelmiller Nov 20, 2023
d0c4238
Implement peekRender with this.getCurrentRender()
jerelmiller Nov 20, 2023
28d24c8
Fix typo in type name
jerelmiller Nov 20, 2023
d1ec7f9
Add a setSnapshot function to set partial data on snapshot
jerelmiller Nov 20, 2023
dbc20fd
Update some uses of updateSnapshot with setSnapshot in useBackgroundQ…
jerelmiller Nov 20, 2023
737b5b2
Destructure in setSnapshot
jerelmiller Nov 21, 2023
1f0d2bd
Update getCurrentRender to use the previous iterator position.
jerelmiller Nov 21, 2023
88e2e47
Update test to use peekRender to get execute function
jerelmiller Nov 21, 2023
d74c691
Minor tweak to error message
jerelmiller Nov 21, 2023
40a47f8
Remove unused currentRender variable
jerelmiller Nov 21, 2023
c29eb1d
Rename setSnapshot to mergeSnapshot and updateSnapshot to replaceSnap…
jerelmiller Nov 21, 2023
874acf6
Merge branch 'main' into jerel/updates-to-profiler
jerelmiller Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,7 @@ function renderPaginatedIntegrationTest({
}

function SuspenseFallback() {
ProfiledApp.updateSnapshot((snapshot) => ({
...snapshot,
ProfiledApp.setSnapshot((snapshot) => ({
suspenseCount: snapshot.suspenseCount + 1,
}));
return <div>loading</div>;
Expand All @@ -450,8 +449,7 @@ function renderPaginatedIntegrationTest({
}) {
const { data, error } = useReadQuery(queryRef);
// count renders in the child component
ProfiledApp.updateSnapshot((snapshot) => ({
...snapshot,
ProfiledApp.setSnapshot((snapshot) => ({
count: snapshot.count + 1,
}));
return (
Expand Down Expand Up @@ -504,8 +502,7 @@ function renderPaginatedIntegrationTest({
<ErrorBoundary
fallback={<div>Error</div>}
onError={(error) => {
ProfiledApp.updateSnapshot((snapshot) => ({
...snapshot,
ProfiledApp.setSnapshot((snapshot) => ({
errorCount: snapshot.errorCount + 1,
errors: snapshot.errors.concat(error),
}));
Expand Down
67 changes: 37 additions & 30 deletions src/testing/internal/profile/profile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,26 @@ export interface NextRenderOptions {
export interface ProfiledComponent<Props, Snapshot>
extends React.FC<Props>,
ProfiledComponentFields<Props, Snapshot>,
ProfiledComponenOnlyFields<Props, Snapshot> {}
ProfiledComponentOnlyFields<Props, Snapshot> {}

interface UpdateSnapshot<Snapshot> {
(newSnapshot: Snapshot): void;
(updateSnapshot: (lastSnapshot: Readonly<Snapshot>) => Snapshot): void;
}

interface ProfiledComponenOnlyFields<Props, Snapshot> {
interface SetSnapshot<Snapshot> {
(partialSnapshot: Partial<Snapshot>): void;
(
updatePartialSnapshot: (
lastSnapshot: Readonly<Snapshot>
) => Partial<Snapshot>
): void;
}

interface ProfiledComponentOnlyFields<Props, Snapshot> {
// Allows for partial updating of the snapshot by shallow merging the results
setSnapshot: SetSnapshot<Snapshot>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a setSnapshot method which allows us now to set a partial snapshot, which gets merged with the full snapshot. I took the name from React's old this.setState() method in class components, which has similar behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment above - we might also consider the current behaviour, but immer-wrapped.

// Performs a full replacement of the snapshot
updateSnapshot: UpdateSnapshot<Snapshot>;
}
interface ProfiledComponentFields<Props, Snapshot> {
Expand All @@ -54,21 +66,14 @@ interface ProfiledComponentFields<Props, Snapshot> {
*/
takeRender(options?: NextRenderOptions): Promise<Render<Snapshot>>;
/**
* Returns the current render count.
* Returns the total number of renders.
*/
currentRenderCount(): number;
totalRenderCount(): number;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now the only function that doesn't rely on the iterator position in its implementation. If possible, we should keep it that way to try and maintain the intent of the original API which should force you to iterate on each render in your tests.

/**
* Returns the current render.
* @throws {Error} if no render has happened yet
*/
getCurrentRender(): Render<Snapshot>;
/**
* Iterates the renders until the render count is reached.
*/
takeUntilRenderCount(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaved in a way that was unintuitive due to the fact that it used the total render count rather than the iterator in its implementation. This isn't currently used, so I'd like us to try using the other methods first before determining if we need to reintroduce this.

If we do reintroduce, I'd propose that instead of thinking about taking each render up to a specific count, that instead this method advances the iterator a certain number of renders from "current". Doing so would prevent weird situations like this:

await Profiled.takeRender()
await Profiled.takeRender()
await Profiled.takeRender()

// This makes no sense since render #2 is now in the past
await Profiled.takeUntilRenderCount(2)

count: number,
optionsPerRender?: NextRenderOptions
): Promise<void>;
/**
* Waits for the next render to happen.
* Does not advance the render iterator.
Expand Down Expand Up @@ -118,6 +123,16 @@ export function profile<
snapshotRef.current = snap;
}
};

const setSnapshot: SetSnapshot<Snapshot> = (partialSnapshot) => {
updateSnapshot((snapshot) => ({
...snapshot,
...(typeof partialSnapshot === "function"
? partialSnapshot(snapshot)
: partialSnapshot),
}));
};

const profilerOnRender: React.ProfilerOnRenderCallback = (
id,
phase,
Expand Down Expand Up @@ -179,28 +194,24 @@ export function profile<
),
{
updateSnapshot,
} satisfies ProfiledComponenOnlyFields<Props, Snapshot>,
setSnapshot,
} satisfies ProfiledComponentOnlyFields<Props, Snapshot>,
{
renders: new Array<
| Render<Snapshot>
| { phase: "snapshotError"; count: number; error: unknown }
>(),
currentRenderCount() {
totalRenderCount() {
return Profiled.renders.length;
},
async peekRender(options: NextRenderOptions = {}) {
if (iteratorPosition < Profiled.renders.length) {
const render = Profiled.renders[iteratorPosition];
if (render.phase === "snapshotError") {
throw render.error;
}
return render;
return this.getCurrentRender();
}
const render = Profiled.waitForNextRender({
return Profiled.waitForNextRender({
[_stackTrace]: captureStackTrace(Profiled.peekRender),
...options,
});
return render;
},
async takeRender(options: NextRenderOptions = {}) {
let error: unknown = undefined;
Expand All @@ -222,15 +233,12 @@ export function profile<
if (!currentRender) {
throw new Error("Has not been rendered yet!");
}
return currentRender;
},
async takeUntilRenderCount(
count: number,
optionsPerRender?: NextRenderOptions
) {
while (Profiled.renders.length < count) {
await Profiled.takeRender(optionsPerRender);

const render = Profiled.renders[iteratorPosition];
if (render.phase === "snapshotError") {
throw render.error;
}
return render;
},
waitForNextRender({
timeout = 1000,
Expand Down Expand Up @@ -322,7 +330,7 @@ export function profileHook<ReturnValue extends ValidSnapshot, Props>(
},
{
renders: ProfiledComponent.renders,
currentSnapshotCount: ProfiledComponent.currentRenderCount,
totalSnapshotCount: ProfiledComponent.totalRenderCount,
async peekSnapshot(options) {
return (await ProfiledComponent.peekRender(options)).snapshot;
},
Expand All @@ -332,7 +340,6 @@ export function profileHook<ReturnValue extends ValidSnapshot, Props>(
getCurrentSnapshot() {
return ProfiledComponent.getCurrentRender().snapshot;
},
takeUntilSnapshotCount: ProfiledComponent.takeUntilRenderCount,
async waitForNextSnapshot(options) {
return (await ProfiledComponent.waitForNextRender(options)).snapshot;
},
Expand Down
6 changes: 3 additions & 3 deletions src/testing/matchers/ProfiledComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ export const toRenderExactlyTimes: MatcherFunction<
const hint = this.utils.matcherHint("toRenderExactlyTimes");
let pass = true;
try {
if (profiled.currentRenderCount() > times) {
if (profiled.totalRenderCount() > times) {
throw failed;
}
try {
while (profiled.currentRenderCount() < times) {
while (profiled.totalRenderCount() < times) {
await profiled.waitForNextRender(options);
}
} catch (e) {
Expand Down Expand Up @@ -84,7 +84,7 @@ export const toRenderExactlyTimes: MatcherFunction<
return (
hint +
` Expected component to${pass ? " not" : ""} render exactly ${times}.` +
` It rendered ${profiled.currentRenderCount()} times.`
` It rendered ${profiled.totalRenderCount()} times.`
);
},
};
Expand Down