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

Update types to support more generic usage #471

Merged
merged 16 commits into from
May 3, 2024

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented May 1, 2024

Fixes #408

Relatively light type clean up for v4. Things are looking pretty good and I ended up not changing that much, which is great. For many use cases there won't be breaking changes here in practice (e.g. if you already had a LCPMetric specifically, the name of the base interface or a union it might belong to doesn't matter).

It's possible some things still went too far for some sensibilities; let me know and I can just roll those back. Once this is looking good, I'll update the README entries around these.

Changes:

  • Switch MetricType for Metric (as originally in Make bindReporter generic over metric type #359)

    Most of the time as a user you just want to know which of the union of possible metric types you have, you don't want the combined type in the old Metric. If you do somehow end up with a generic Metric, it's now a discriminated union, so checking e.g. metric.name === 'LCP' will narrow the type to LCPMetric. The old Metric is still available as MetricBase (open for name bike shedding)

  • Ditch CLSReportCallback/CLSReportCallbackWithAttribution, etc. Maybe controversial, but IMO these end up as too many named types for things users will usually never explicitly type, and the only important part is the parameter type, so let's just use that directly.

  • slight clean up of unattributedOn* methods

    e.g. unattributedOnCLS was supposedly calling back with a metric of type CLSMetricWithAttribution, which isn't true. metric is a CLSMetric and only becomes a CLSMetricWithAttribution after attributeCLS is called. Still a type assertion as CLSMetricWithAttribution in there, but it better reflects what's really going on. We could have the attribute* methods return the newly attributed object to avoid the extra type assertion, but I wasn't sure if that was too big a change.

  • A few scattered removed unnecessary type assertions, e.g. as PerformanceEventTiming when the compiler already knows they're PerformanceEventTimings

@brendankenny
Copy link
Member Author

We could have the attribute* methods return the newly attributed object to avoid the extra type assertion, but I wasn't sure if that was too big a change.

I like this way better, actually, so I added it. Separate commit so it's easy to remove, though.

src/attribution/onFCP.ts Outdated Show resolved Hide resolved
src/attribution/onLCP.ts Show resolved Hide resolved
Copy link
Member

@philipwalton philipwalton left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this @brendankenny! For the most part these changes LGTM. The main thing missing right now is to copy the type changes to the relevant parts of the README documentation.

src/attribution/onFCP.ts Outdated Show resolved Hide resolved
src/attribution/onFCP.ts Outdated Show resolved Hide resolved
src/types/base.ts Outdated Show resolved Hide resolved
src/types/base.ts Outdated Show resolved Hide resolved
src/attribution/onLCP.ts Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member Author

The main thing missing right now is to copy the type changes to the relevant parts of the README documentation

PTAL

src/attribution/onCLS.ts Outdated Show resolved Hide resolved
src/attribution/onLCP.ts Outdated Show resolved Hide resolved
@philipwalton
Copy link
Member

@brendankenny I made some updates to your updates (and merged in @tunetheweb's test fixes). Let me know if you have any concerns with any of the changes I made.

Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

LGTM with some README link suggestions

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@tunetheweb
Copy link
Member

Oh and just realised there's a merge conflict due to #468 being merged.

philipwalton and others added 2 commits May 3, 2024 06:51
Co-authored-by: Barry Pollard <barrypollard@google.com>
@philipwalton
Copy link
Member

Actually I ended up just removing all the links to the source types because it looks like they no longer work with GitHub's new file viewer UI.

@philipwalton philipwalton merged commit fd8b0c3 into GoogleChrome:v4 May 3, 2024
6 checks passed
@philipwalton philipwalton changed the title Small type clean up for v4 Update types to support more generic usage May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants