Skip to content

URL Metric mutation helpers should be exposed in initialize args to match finalize args #1930

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

Closed
westonruter opened this issue Mar 16, 2025 · 2 comments · Fixed by #1951
Closed
Assignees
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@westonruter
Copy link
Member

westonruter commented Mar 16, 2025

As discussed in #1928 (comment):

(Granted, this might be able to be refactored so that it happens at initialize(), so that in the handleLCPMetric() function it calls the extendRootData() function every time. This would depend on passing extendRootData() and extendElementData() to initialize() and we'd need to define urlMetric earlier. This would allow Image Prioritizer to get rid of its externalBackgroundImages variable since it would be directly putting the LCP data into the urlMetric whenever a new one is detected.)

This becomes more important when considering that if the browser is asked to do too much work during pagehide it may terminate execution of the function. See #1928. So the earlier we amend the URL Metric data the better.

These are the relevant properties of FinalizeArgs:

readonly getRootData: () => URLMetric;
readonly extendRootData: ( properties: ExtendedRootData ) => void;
readonly getElementData: ( xpath: string ) => ElementData | null;
readonly extendElementData: (
xpath: string,
properties: ExtendedElementData
) => void;

We need to include them in InitializeArgs.

Then we'll need to move the client extension initialization logic down to occur after:

urlMetric = {
url: currentUrl,
viewport: {
width: win.innerWidth,
height: win.innerHeight,
},
elements: [],
};

Aside:

Let's add logging for each extension that is loaded.

We should also remove this old log entry (cf. d2e266d):

log( 'Detection is stopping.' );

Lastly, let's move this line up to a more logical spot to right after it is last used (see 4025e61, although it should be placed right after disconnectIntersectionObserver()):

// Clean up.
breadcrumbedElementsMap.clear();

Finally, we should deprecate finalize because we cannot reliably compress the URL Metric data at pagehide.

@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2025 Mar 16, 2025
@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Mar 16, 2025
@westonruter westonruter moved this from Not Started/Backlog 📆 to To Do 🔧 in WP Performance 2025 Mar 16, 2025
@westonruter
Copy link
Member Author

Embed Optimizer can be updated to use extendElementData() from initialize() as well. Instead of adding the resize observer in from initialize and then using this to populate a loadedElementContentRects which then gets passed in extendElementData() in finalize():

/**
* Finalizes extension.
*
* @type {FinalizeCallback}
* @param {FinalizeArgs} args Args.
*/
export async function finalize( {
log: _log,
error: _error,
getElementData,
extendElementData,
} ) {
/* eslint-disable no-console */
// TODO: Remove once Optimization Detective likely updated, or when strict version requirement added in od_init action.
const log = _log || console.log;
const error = _error || console.error;
/* eslint-enable no-console */
for ( const [ xpath, domRect ] of loadedElementContentRects.entries() ) {
try {
extendElementData( xpath, {
resizedBoundingClientRect: domRect,
} );
const elementData = getElementData( xpath );
log(
`boundingClientRect for ${ xpath } resized:`,
elementData.boundingClientRect,
'=>',
domRect
);
} catch ( err ) {
error(
`Failed to extend element data for ${ xpath } with resizedBoundingClientRect:`,
domRect,
err
);
}
}
}

We can instead just call extendElementData() in the resize observer directly in the callback. This will eliminate additional work done during pagehide.

@westonruter
Copy link
Member Author

As noted in #1893 (comment), I think we should consider eliminating the finalize API from extensions. It is somewhat dangerous because it could add delays or compute that causes execution of the pagehide event handler to be interrupted.

@ShyamGadde ShyamGadde self-assigned this Mar 21, 2025
@westonruter westonruter moved this from To Do 🔧 to In Progress 🚧 in WP Performance 2025 Mar 21, 2025
@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done 😃 in WP Performance 2025 Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
Status: Done 😃
Development

Successfully merging a pull request may close this issue.

2 participants