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

Removed promise antipattern from code example #227

Closed
wants to merge 1 commit into from
Closed

Removed promise antipattern from code example #227

wants to merge 1 commit into from

Conversation

bergus
Copy link
Contributor

@bergus bergus commented Mar 1, 2016

See What is the explicit promise construction antipattern and how do I avoid it? on StackOverflow.
Promise evangelists say it should be burned with fire, and not appear in any possibly educational code examples :-)

@annevk
Copy link
Member

annevk commented Mar 1, 2016

Thank you @bergus, this indeed seems like something we should get right. @domenic, could you review this please?

@jakearchibald
Copy link
Collaborator

This looks correct to me. Dunno if there's a style that should be followed regarding semicolons, but the use of promises seems correct.

@annevk
Copy link
Member

annevk commented Mar 1, 2016

@bergus, could you add yourself to the acknowledgments section?

@domenic
Copy link
Member

domenic commented Mar 1, 2016

This was discussed a bit during the merge. There is a slight problem here, which is that the form @bergus advocates will cause memory to be consumed for the entire chain of promises (i.e. the first chunk's promise cannot be GCed until the last chunk's promise is GCed, which does not happen until the stream is closed/errored). This is a design flaw/bug of promises, discussed a lot previously in tc39/proposal-cancelable-promises#1 and linked places.

I am on the fence on this, but maybe it is better to advocate the "leaky" pattern in the hopes that we will one day fix it/optimize it in implementations?

@jakearchibald
Copy link
Collaborator

What leaks here? Just the promise object, or the chunks too?

@domenic
Copy link
Member

domenic commented Mar 1, 2016

Just promise objects. The individual promises fulfilled with each chunk are not retained. Instead it's just a long chain of pending promises resolved to the next item in the chain, until the last promise is finally fulfilled.

@jakearchibald
Copy link
Collaborator

I think that's probably ok in this case. Hopefully implementations can squash that chain?

@bergus
Copy link
Contributor Author

bergus commented Mar 4, 2016

Ah yeah the recursive promise chain memory problem.
Imo, this flaw should be fixed in the spec and in implementations.
@domenic I'm all for advocating the proper pattern. Its use should be encouraged, and if that puts pressure on implementations to fix the leak then mission accomplished :-) The antipattern with .then(resolve, reject) also prevents propagation of cancellation and similar things.
@jakearchibald I found the semicolon style weird as well, but just didn't want to change anything here.
@annevk Is my full real name required there?

@domenic
Copy link
Member

domenic commented Mar 4, 2016

Yeah, it's unfortunate that it's not a trivial fix (since the easy fix breaks the ability for subclasses to customize .then behavior).

@annevk
Copy link
Member

annevk commented Mar 4, 2016

@bergus no, it's completely up to you and only if you want to be listed. No requirements.

@annevk
Copy link
Member

annevk commented Mar 8, 2016

@bergus ping.

annevk pushed a commit that referenced this pull request Mar 9, 2016
PR: #227

(Anne also snuck in a comment that he forgot to add in the previous commit.)
@annevk
Copy link
Member

annevk commented Mar 9, 2016

Thank you, landed as af4664d. Assumed you did not want to be listed at all.

@annevk annevk closed this Mar 9, 2016
@bergus
Copy link
Contributor Author

bergus commented Mar 9, 2016

Thanks, it's fine. If I'll change my mind and want to be acknowledged, I'll find something else to contribute :-)

@bergus bergus deleted the patch-1 branch March 9, 2016 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants