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

Enable eslint rule and reject with errors instead of strings #42

Merged

Conversation

kox
Copy link
Contributor

@kox kox commented Dec 15, 2024

Following #13 , this PR intends to enable the eslint rule: prefer-promise-reject-errors as error.

I believe that treating promise rejections as errors ensures better handling and propagation of the rejection reason to the final components. The affected package tests have been updated accordingly.

Copy link

changeset-bot bot commented Dec 15, 2024

⚠️ No Changeset found

Latest commit: 204f681

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

@github-actions github-actions bot added the stale label Dec 30, 2024
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

The reason property of an AbortSignal can actually be an Error.

Consider the difference between:

const ac = new AbortController();
ac.signal.addEventListener('abort', function () { console.log(this.reason.constructor, this.reason); })
ac.abort();

…and…

const ac = new AbortController();
ac.signal.addEventListener('abort', function () { console.log(this.reason.constructor, this.reason); })
ac.abort('o no');

reason is a DOMException in the first, and a String in the second.

I think there are only two ways forward here:

  • Leave this as is, suppress the lint.
  • If the reason is typeof string then wrap it in an Error. Otherwise, reject with reason directly.

I think the second raises more questions than it answers, so I'm inclined to do the first.

@github-actions github-actions bot removed the stale label Jan 3, 2025
@kox kox force-pushed the enable-eslint-prefer-promise-reject-errors branch from a6715b3 to f59b494 Compare January 4, 2025 21:34
@kox
Copy link
Contributor Author

kox commented Jan 4, 2025

Following your concerns and for simplicity, I suppressed the lints and introduced Errors inside of a test and an example.

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

I still think the Error conversions need to go.

examples/rpc-transport-throttled/src/example.ts Outdated Show resolved Hide resolved
packages/rpc/src/__tests__/rpc-request-coalescer-test.ts Outdated Show resolved Hide resolved
@kox kox force-pushed the enable-eslint-prefer-promise-reject-errors branch from f59b494 to 204f681 Compare January 20, 2025 13:35
@kox
Copy link
Contributor Author

kox commented Jan 20, 2025

Sorry about that. I took a deeper look into the AbortController and AbortSignal and you are right about it. It doesn't make sense to wrap it in an extra Error.

Copy link
Collaborator

@steveluscher steveluscher 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 improving our code quality!

@steveluscher steveluscher disabled auto-merge January 21, 2025 18:33
@steveluscher steveluscher merged commit 8053823 into anza-xyz:main Jan 21, 2025
2 checks passed
@kox kox deleted the enable-eslint-prefer-promise-reject-errors branch January 22, 2025 08:28
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.

2 participants