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

chore: let @typescript-eslint/no-floating-promises ESLint rule pass #6331

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented May 6, 2023

Summary of changes

This is just the relevant parts of the code changes from #6325 instead of also tackling eslint-config changes.

  • Added void operator to Promises that are not being awaited. Used await on finally retry in test.

Details

When enabling the typescript-eslint rules contained in "plugin:@typescript-eslint/recommended-requiring-type-checking" I tried to improve code quality by addressing some of the issues raised when enabling the type checked linting rules.

This change specifically resolves issues raised with the typescript-eslint error: no-floating-promises.

error Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator @typescript-eslint/no-floating-promises

Testing Strategy:

I opened up my windows machine and ran this script (with the additional eslint rules enabled from #6325):

yarn lint
yarn test:primary
npx playwrite install
yarn test
yarn build --tsc

The specific rule configuration was:

  "@typescript-eslint/no-floating-promises": [
    "error",
    { ignoreVoid: true, ignoreIIFE: true },
  ],

@changeset-bot
Copy link

changeset-bot bot commented May 6, 2023

⚠️ No Changeset found

Latest commit: c4a362c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

I don't think this needs a changeset since it's internal cosmetics

@MichaelDeBoey MichaelDeBoey changed the title Fix: Code quality issues of No floating promises chore: let @typescript-eslint/no-floating-promises ESLint rule pass May 6, 2023
.changeset/tough-roses-admire.md Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey added needs-response We need a response from the original author about this issue/PR feat:typescript labels May 6, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 6, 2023
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented May 6, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@brophdawg11 brophdawg11 requested review from pcattori and removed request for brophdawg11 May 15, 2023 16:17
@pcattori
Copy link
Contributor

pcattori commented Jul 5, 2023

#6325 was closed w/o merging in favor of #6248 , so (I think) this PR is obsolete now. @ngbrown let me know if there's a reason to reopen it.

@pcattori pcattori closed this Jul 5, 2023
@ngbrown
Copy link
Contributor Author

ngbrown commented Jul 5, 2023

@pcattori This made code changes that #6248 didn't make, specifically the fix to withApp.ts#L39 is a test runtime behavior improvement.

There were no eslint rule changes in this pull request. I think this pull request should be re-opened.

@JoshuaKGoldberg
Copy link
Contributor

@MichaelDeBoey just checking in, is there anything else left on this PR?

I'm working on the other recommended typed rules from typescript-eslint and saw this one is still hanging around.

Copy link
Contributor

@pcattori pcattori left a comment

Choose a reason for hiding this comment

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

LGTM, just needs conflicts with dev to be resolved

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Aug 7, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Aug 8, 2023
@ngbrown
Copy link
Contributor Author

ngbrown commented Aug 8, 2023

I've rebased the branch, re-ran the no-floating-promises rule, and updated where there were new errors.

@brophdawg11
Copy link
Contributor

Sorry it looks like this has new conflicts 😬 . I merged dev in locally but don't have access right to push to your fork

error  Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator  @typescript-eslint/no-floating-promises
error  Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator  @typescript-eslint/no-floating-promises
@ngbrown
Copy link
Contributor Author

ngbrown commented Aug 9, 2023

@brophdawg11 I re-resolved the conflicts and made sure formatting was applied to components.tsx this time.

Because I forked into a separate org, to organize my GitHub forks, I don't have the option to enable maintainers to push to the branch. I didn't realize that was a drawback at the time.

@brophdawg11 brophdawg11 merged commit ff45f1d into remix-run:dev Aug 9, 2023
9 checks passed
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-b1149bb-20230810 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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.

6 participants