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

Avoid no-floating-promises ESLint problem #11794

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

karlhorky
Copy link

Hi there!

First of all, thanks for all the amazing work that goes into maintaining create-react-app and react-scripts. We've seen a lot of success with teaching it at @upleveled.

Recently came across @​typescript-eslint/no-floating-promises (from tweets like https://mobile.twitter.com/flybayer/status/1469098848958222337) and it has problems with the code in reportWebVitals.js

Screen Shot 2021-12-17 at 19 54 40

This avoids the issue without changing the functionality

@karlhorky
Copy link
Author

By the way, also happy to add a proper catch callback here, if it's so wished.

@raix raix added this to the 5.0.1 milestone Dec 21, 2021
getLCP(onPerfEntry);
getTTFB(onPerfEntry);
})
.catch(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good fix for this issue but if there is a real error with the import it will be swallowed. Can you catch and log any error that might happen instead?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, for sure, I'll just use console.error then

Copy link
Author

Choose a reason for hiding this comment

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

Done in 39a4695

@iansu iansu modified the milestones: 5.0.1, 5.0.2 Apr 12, 2022
@karlhorky
Copy link
Author

@iansu any further feedback on this PR? Do you think we could get it merged / released?

@karlhorky
Copy link
Author

An alternative to this PR is the following PR, which removes web-vitals entirely:

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