-
Notifications
You must be signed in to change notification settings - Fork 415
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
3.3.2: type errors using attribution callbacks #351
Comments
Could you please explain how you're seeing this error? I tried creating a new project with the above example snippet in a $ 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
$ |
@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 This is in a brand new project with nothing but the latest versions of 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;
}); |
OK so it appears you can't just change a type of an interface in Typescript by repeating it. By removing the 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? |
That fixes it! 🚀 |
@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? |
Just checked, it still works 😄 Thank you! |
Following the fixes for #331 in #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:
Suggested fix:
We would need to make a similar change for the other metrics as well.
The text was updated successfully, but these errors were encountered: