GoogleChrome / web-vitals

Essential metrics for a healthy site.
https://web.dev/vitals
Apache License 2.0
7.59k stars 415 forks source link

3.3.2: type errors using attribution callbacks #351

Closed OliverJAsh closed 1 year ago

OliverJAsh commented 1 year ago

Following the fixes for https://github.com/GoogleChrome/web-vitals/issues/331 in https://github.com/GoogleChrome/web-vitals/pull/348, I upgraded to 3.3.2 to see if it fixed the issues I was facing, however I am still having problems. For example:

import { onCLS } from "web-vitals/attribution";
onCLS((metric) => {
  // Expected type: `string | undefined`
  // Actual type: `unknown`
  metric.attribution.largestShiftTarget;
  //                 ^?
});

Suggested fix:

-export interface CLSReportCallbackWithAttribution extends ReportCallbackWithAttribution {
+export interface CLSReportCallbackWithAttribution {
     (metric: CLSMetricWithAttribution): void;
 }

We would need to make a similar change for the other metrics as well.

tunetheweb commented 1 year ago

Could you please explain how you're seeing this error? I tried creating a new project with the above example snippet in a test.js file and then run the following:

$ npm install web-vitals@3.3.1

changed 1 package, and audited 3 packages in 573ms

found 0 vulnerabilities
$ tsc --strict test.ts        
\test.ts:5:10 - error TS2339: Property 'attribution' does not exist on type 'Metric | CLSMetric | CLSMetricWithAttribution'.
  Property 'attribution' does not exist on type 'Metric'.

5   metric.attribution.largestShiftTarget;
           ~~~~~~~~~~~

Found 1 error in test.ts:5

The same thing with 3.3.2 does not happen:

$ npm install web-vitals@3.3.2

changed 1 package, and audited 3 packages in 678ms

found 0 vulnerabilities
$ tsc --strict test.ts        
$ 
OliverJAsh commented 1 year ago

@tunetheweb Apologies, there was a typo in my original post. I've corrected it now so hopefully it makes a bit more sense.

To clarify, we're expecting the type of largestShiftTarget to be string | undefined but if you hover over you will see it's unknown.

This is in a brand new project with nothing but the latest versions of typescript and web-vitals.

Here's an adjusted example which generates a type error:

import { onCLS } from "web-vitals/attribution";
onCLS((metric) => {
  // Error: Type 'unknown' is not assignable to type 'string | undefined'.
  const x: string | undefined = metric.attribution.largestShiftTarget;
});
tunetheweb commented 1 year ago

OK so it appears you can't just change a type of an interface in Typescript by repeating it. By removing the extends you only have one metric and TypeScript is able to work properly.

However, that causes other problems (web-vitals.js will not build) so the answer appears to be a little trickier.

Can you try the branch from this PR and let me know if that works? https://github.com/GoogleChrome/web-vitals/pull/356

OliverJAsh commented 1 year ago

That fixes it! 🚀

tunetheweb commented 1 year ago

@OliverJAsh I updated the PR with your original suggestion. Not sure why couldn't get your suggestion to work previously but could you double check that branch still works to solve your issue before I merge this to save us having a third attempt at this?

OliverJAsh commented 1 year ago

Just checked, it still works 😄 Thank you!