Skip to content

feat: better naming, typedef, wrap#1

Open
ThaUnknown wants to merge 1 commit intoJenniferPylko:mainfrom
ThaUnknown:main
Open

feat: better naming, typedef, wrap#1
ThaUnknown wants to merge 1 commit intoJenniferPylko:mainfrom
ThaUnknown:main

Conversation

@ThaUnknown
Copy link

I wanted to make something like this, found your library, decided to contribute

What this PR does:

  • better naming: there's no need to name the async constructor async_constructor, if its already gonna be prefixed with async because you then have async async_constructor, just change it to constructor and we get async [constructor], miles easier to read
  • added a wrap method, which uses the Proxy class, this is useful when u can't extend the async class, but I didn't test it against your asyncSuper thing
  • added type definitions, so when you go to use the class or wrap function, instellisense will say "hey this constructor actually returns a promise", nice for type safety

@JenniferPylko
Copy link
Owner

thank you so much for the pr! i have a few notes, but overall it looks pretty good.

  • in your typedef you still refer to the class as AsyncClass, was this intentional? i'm a bit rusty on typescript so i may just be misunderstanding
  • i'm a bit wary of using a keyword as a variable name, even if it's ostensibly safe, but i do like the elegance of async [constructor]. to make it consistent with Super, i'd say let's name it Constructor.

some style things (i guess i should write up a style guide for contributing huh)

  • please revert the whitespace change on the prototype generator function (function*( vs function * ()
  • please use double quotes instead of single quotes
  • please use snake case for non-exported, non-member variables
  • please make wrap() an arrow function

i'll add some inline comments for a couple other things too, but thank you again!

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