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

browser: refactor/remove asPromise #686

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Conversation

silesky
Copy link
Contributor

@silesky silesky commented Nov 19, 2022

This asPromise method seems to add unneccessary complexity both in the way it's used and by virtue of its existence. If there's a galaxy brain reason for why we'd do this, I can't find it.

A util method that's just an alias to Promise.resolve?... like whyyyyy 😄 And there are actual unit tests for this method?

@changeset-bot
Copy link

changeset-bot bot commented Nov 19, 2022

⚠️ No Changeset found

Latest commit: 04bf2c6

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

@silesky silesky requested a review from chrisradek November 19, 2022 22:03
@silesky silesky force-pushed the refactor/remove-as-promise branch from e4106b8 to 2a17b84 Compare November 19, 2022 22:05
@silesky silesky requested a review from a team November 19, 2022 22:06
},
})
)
await fn({
Copy link
Contributor Author

@silesky silesky Nov 19, 2022

Choose a reason for hiding this comment

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

I assume that the original intention to use await asPromise here was because MiddlewareFunction and SourceMiddleware could return a Promise -- and maybe whoever wrote this originally didn't realize that void | Promise<void> is a valid return type -- or that there's no requirement to "normalize" all values to a promise in order to await -- that you can just as easily await a regular value as a promise (await 123=== await Promise.resolve(123)).

@silesky silesky changed the title remove as-promise Refactor -- remove as-promise code smell Nov 19, 2022
@silesky silesky changed the title Refactor -- remove as-promise code smell Refactor -- remove unneeded Promise coercion Nov 19, 2022
@silesky silesky changed the title Refactor -- remove unneeded Promise coercion refactor to remove unneeded Promise coercion Nov 19, 2022
@silesky silesky requested a review from ryder-wendt November 20, 2022 06:59
@silesky silesky force-pushed the refactor/remove-as-promise branch from 2a17b84 to f81ed0c Compare November 20, 2022 07:10
@silesky silesky force-pushed the refactor/remove-as-promise branch from f81ed0c to 04bf2c6 Compare November 20, 2022 07:18
@silesky silesky changed the title refactor to remove unneeded Promise coercion refactor to remove asPromise Nov 20, 2022
@silesky silesky changed the title refactor to remove asPromise refactor: remove asPromise Nov 20, 2022
@silesky silesky changed the title refactor: remove asPromise browser: refactor/remove asPromise Nov 27, 2022
@silesky silesky force-pushed the master branch 2 times, most recently from da031b5 to 275d5a3 Compare November 27, 2022 23:54
@silesky silesky requested a review from pooyaj November 28, 2022 22:41
Copy link
Contributor

@chrisradek chrisradek left a comment

Choose a reason for hiding this comment

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

Sweet, love removing code!

@silesky silesky merged commit c5d2a99 into master Dec 2, 2022
@silesky silesky deleted the refactor/remove-as-promise branch December 2, 2022 22:11
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.

3 participants