Skip to content

Async support + various fixes#1

Merged
Mcsavvy merged 25 commits intoMcsavvy:mainfrom
joeycumines-swyftx:main
Jul 8, 2022
Merged

Async support + various fixes#1
Mcsavvy merged 25 commits intoMcsavvy:mainfrom
joeycumines-swyftx:main

Conversation

@joeycumines-swyftx
Copy link
Contributor

I was evaluating your implementation, and made these changes while doing so.

I'd say I'm happy with the concept, and fairly happy with the API it provides. IMO, the biggest issue is that w/o async support this package has only limited usefulness.

Features:

  • Added AsyncContextManager, as a superset of ContextManager (with the same two methods / behavior)
  • Refactored With to support return values from the body
  • Added AsyncWith Same thing, but async
  • Added AsyncUse and Use (context manager to generator, as a more-limited alternative to With)
  • Refactored closing to (also) support async context managers

Fixes / tweaks:

  • Only suppress if exit actually returned true, not just truthy
  • Stripped console.log calls (immediate deal-breaker, as someone evaluating a package)
  • Identify the error case (within implementations of / that call exit) via arguments.length (undefined can be thrown)
  • Added tsconfig, linting etc

Type fixes / tweaks:

  • Made this package build (with our TS config), by addressing the incorrect assumption that only Error values will ever be thrown
  • Fixed/tweaked various signatures (I made an effort to make them all compatible, but I have no reliable way to verify this)
  • Deprecated the interfaces that I could not see any value in exporting
  • Deleted the unexported body type (that was used in one place)

@Mcsavvy Mcsavvy merged commit 7209bfa into Mcsavvy:main Jul 8, 2022
@Mcsavvy
Copy link
Owner

Mcsavvy commented Jul 8, 2022

Nice fixing

@Mcsavvy
Copy link
Owner

Mcsavvy commented Jul 8, 2022

I was working on separate branches for ts and js

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