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.

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

WIP: Add snapshot support to byte efficiency audits #12684

Closed
wants to merge 1 commit into from

Conversation

adamraine
Copy link
Member

This PR adds split functionality to ByteEfficiency audits with a new function auditSnapshot_. For this to be possible, we need a way to make timespan/navigation specific artifacts optional.

The current implementation makes all artifacts optional when supportedModes is specified. This works fine, but I'm open to adding a more explicit artifactsAreOptional flag or something.

As an example, I added snapshot support to uses-responsive-images:

Screen Shot 2021-06-23 at 12 43 41 PM

@patrickhulce
Copy link
Collaborator

Adam and I chatted about this offline, and we're going to continue discussion about some of the different paths forward in a doc.

A few things we called out:

  • Option w/ splitting the audit into two
  • Option w/ optionalArtifacts
  • Option w/ navigationArtifacts/snapshotArtifacts
  • How does this play into the opportunity rethink and the structure of opportunity details missing KB/overallSavingsMs?

@adamraine
Copy link
Member Author

I started to break down each option in a separate doc, but I thought it might be overkill:

  1. Add an option to the audit meta which makes every artifact in requiredArtifacts optional
    • This is basically what is implemented right now, artifacts become optional when supportedModes is defined.
    • The existence of artifacts must be manually asserted in the audit implementation.
  2. Add an optionalArtifacts property to the audit meta:
    • Allows us to only make specific artifacts optional
    • The existence of optional artifacts must be asserted in the audit implementation
  3. Add navigationArtifacts/snapshotArtifacts to the audit meta
    • Required artifacts are explicit for each mode
    • Basically splitting into two audits, but it’s all done in the same file
    • Could elevate auditSnapshot to be available on all audits, not just byte efficiency
  4. Split into completely separate audits
    • Snapshot audit doesn’t need to inherit from byte efficiency
    • Sharing functionality is more inconvenient, will need to import ui strings as well
    • Will end up with a bunch of duplicate files like uses-responsive-images-snapshot.js

The biggest problem with 1 and 2 is that we don't need such a loose structure to add snapshot support. We are either using the entire audit implementation, or falling back to a smaller implementation in snapshot mode. For this reason, I'm currently leaning towards options 3 and 4.

@brendankenny @connorjclark @paulirish any thoughts on the 4 approaches?

@patrickhulce
Copy link
Collaborator

Split into completely separate audits

I think I'm leaning toward this option for now as well. It's the only choice that allows us to accomplish everything we need for MVP while leaving us the most flexibility to restructure/harden a different API as we learn more. The others smell a bit like YAGNI at this point in time :)

@brendankenny
Copy link
Member

@brendankenny @connorjclark @paulirish any thoughts on the 4 approaches?

Simpler and more explicit is always music to my ears, so option 4 until we have a good lay of the land sounds great to me

@adamraine
Copy link
Member Author

Splitting into separate audits seems like the best approach for now. We can come back to this if we find a good reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants