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

Add promises #202

Closed
wants to merge 4 commits into from
Closed

Add promises #202

wants to merge 4 commits into from

Conversation

hayd
Copy link
Contributor

@hayd hayd commented Feb 18, 2019

Move deferred from util/ to promises/.

Add delay and timeout promises.

cc @ry @keroxp #188 (also denoland/deno#1800 #201 etc.)

Note: This would also need promises to be added to the registry: https://github.com/denoland/registry/pull/57

I am sure there are several other useful promise constructs, but these were the first that came to mind.

hayd added a commit to hayd/registry that referenced this pull request Feb 18, 2019
promises/test.ts Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented Feb 18, 2019

What else is going to go into promises?

Can you explain why you're against "util"?

@hayd
Copy link
Contributor Author

hayd commented Feb 19, 2019

Perhaps a pool implementation (which I have) and likely some other helpers (see e.g. bluebird).

util is not descriptive... at all. IMOT util is a blight (everything's a util 😭 ), naming is really important (and difficult), likely we'll refactor in the future but nevertheless we should make a best effort.

Helpers for promises, as well as for generators (ala itertools), are things that should be in std. I remember there was an issue somewhere to discuss what would be in std - but I can't find it.

@keroxp
Copy link
Contributor

keroxp commented Feb 19, 2019

I have neutral opinion about util. But I don't strongly oppose util module set.
A problem we have to decide at now is how should small modules be placed on. Those modules are usually difficult to be classified.

There are some different solutions:

    1. treating every single small module as a top level module set (like //deferred/mod.ts)
    1. create util module set and move them into util (//util/deferred.ts)
    1. create new module sets for them and strictly classify it (//promises/deferred.ts)

I don't agree with 1, and 3 may become difficult to manage as time goes by.
I'm sure 2 is not best solution but possible one at this time.
There are no plan to use promises or promise for other promise-related modules.

I respect @ry 's decision for this repository. We have to make some decision on #130 together.

And by this case, I'm also concerned about usage of deno_std's namespace in deno.land/x/. I will open another issue...

@hayd
Copy link
Contributor Author

hayd commented Feb 19, 2019

An argument could be made for dropping all the independent std modules in the registry, i.e. reference std solely under /x/std/... I'm not sure about that. (Though it would solve the relative/not found issue.)

This PR is independent of #130. There's certainly more promise-related helpers that could be included to /promises/ (I added a couple already and have some follow up ideas), I don't think anything is gained by having deferred in its own file in this case.

There is a larger question/issue: what else should be in std?

@ry
Copy link
Member

ry commented Feb 19, 2019

I think we ought to remove Deno.land/x/http and other sub modules of std... I will do some work on this today.
I very much agree with @keroxp’s position in these org discussions.

export class TimeoutError extends Error {
constructor(ms: number) {
const message = `Timeout after ${ms}ms`;
super(message);
Copy link

Choose a reason for hiding this comment

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

Could these two lines be combined?

Suggested change
super(message);
super(`Timeout after ${ms}ms`);

Also, should this use something like Node’s ms utility to display the elapsed time in a more friendly manner?

@hayd
Copy link
Contributor Author

hayd commented Feb 20, 2019

@ry should this also be in individual files: promises/deferred.ts promises/timeout.ts promises/delay.ts ??

@ry
Copy link
Member

ry commented Mar 5, 2019

This one is out of date - if you want to land this still - let's discuss on gitter

@ry ry closed this Mar 5, 2019
@hayd
Copy link
Contributor Author

hayd commented Mar 5, 2019

@ry defer is useful (and already used in several projects) and would be good to add back to std somewhere, as are the others...

Happy to re-land this, but want to know how you want it structured.

should this also be in individual files: promises/deferred.ts promises/timeout.ts promises/delay.ts

@hayd hayd deleted the promises branch May 24, 2019 19:51
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.

5 participants