Skip to content

Conversation

@Snapstromegon
Copy link

In this MR I bumped the TS target to ES2015 to support promises.
Also all existing code (less than 100 downloads on NPM) will break with this MR, because it uses the async version instead of the sync ones by default!

This package only provides a synchronous version of each function and especially for the slowAfter this might be a problem because it uses a while loop for sleeping.

This Commit renames the existing functions to their *Sync version and adds async implementations (which most of the time just use the sync one).

It also makes the async ones the default, so a new developer would be encouraged to use those functions instead.

Tests are added to reflect the sync versions and also readme is updated.

This package only provides a synchronous version of each function and especially for the `slowAfter` this might be a problem because it uses a while loop for sleeping.

This Commit renames the existing functions to their *Sync version and adds async implementations (which most of the time just use the sync one).

It also makes the async ones the default, so a new developer would be encouraged to use those functions instead.

Tests are added to reflect the sync versions and also readme is updated.

Signed-off-by: Raphael Höser <[email protected]>
Signed-off-by: Raphael Höser <[email protected]>
}

export function warnAfter(time: Date, options: Partial<Config> = {}) {
export async function warnAfter(time: Date, options: Partial<Config> = {}) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer not to clutter up the lib with async versions of things that shouldn't take time -- given that await undefined === await Promise.resolve(undefined), i'm not sure if there's a use case for this.

Copy link
Author

Choose a reason for hiding this comment

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

My intention here was to have all async versions return a promise to make this lib easier to use.

For the same reason I included a sync and async version for everything.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I get the intention, but I think waitAfter (non-sync) would be the only use case I can imagine for using the return value, so if it comes down to a choice between removing a bunch of duplicate functions where it's unclear why they exist vs returning an instantly resolving promise, I'd choose the former.

Or I'd just modify the originals to return Promise.resolve(undefined) and remove the sync versions entirely, but even that I feel is unnecessary.

Thanks for your input!

}

export function slowAfter(
export async function slowAfter(
Copy link
Owner

Choose a reason for hiding this comment

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

NB: I'd prefer if we managed to figure out a way to share the logic between slowAfter and slowAfterSync-- maybe a generic higher-order-function that returns the result of wait?

function buildSlowAfter<T>(waitFn: ((number) => A)){
  return (time, delay, options) => {
    ...
  }
}

const slowAfter = buildSlowAfter(wait);
const slowAfterSync = buildSlowAfter(waitSync);

Copy link
Author

Choose a reason for hiding this comment

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

If this lib (like right now) doesn't need to do anything after the wait, we could wrap the loop in a promise (since the executer gets executed in the same callstack if I'm not mistaken).

This would make a higher order function easier to implement.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good! Do whatever you think is cleanest -- i just proposed what I thought might work cleanly.

@evinism
Copy link
Owner

evinism commented Nov 23, 2020

Thank you so much for your contribution! Just a few comments :)

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