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

stream docs, writable.cork() and writable.uncork() methods #7340

Closed
italoacasas opened this issue Jun 19, 2016 · 8 comments
Closed

stream docs, writable.cork() and writable.uncork() methods #7340

italoacasas opened this issue Jun 19, 2016 · 8 comments
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.

Comments

@italoacasas
Copy link
Contributor

Working on #7287 I notice that writable.cork() and writable.uncork() are not together, I guess it's because the alphabetical order, but I think the experience reading the stream documentation is going to be better if they live together.

@italoacasas italoacasas changed the title stream docs form, writable.cork() and writable.uncork() methods stream docs, writable.cork() and writable.uncork() methods Jun 19, 2016
@addaleax addaleax added the doc Issues and PRs related to the documentations. label Jun 19, 2016
@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Jun 19, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jun 20, 2016

You're right, but I think the overall doc experience is better if everything is predictably in alphabetical order.

@italoacasas
Copy link
Contributor Author

italoacasas commented Jun 20, 2016

Yes, alphabetical order I think is the right choose, but on my opinion we can improve the experience if we make an exception on this case.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 20, 2016

There are many other cases in the documentation where an exception would be warranted. The problem is that if you keep making exceptions, then the documentation loses all consistency.

@jasnell
Copy link
Member

jasnell commented Jun 20, 2016

I'm -1 on changing the order. Improving the documentation so that the links between the two are clearer is definitely a good idea.

@addaleax
Copy link
Member

How about adding See also: links in the places where it would otherwise make sense to reorder, e.g. one from cork() to uncork() and vice versa?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 21, 2016

Makes sense to me.

@Salakar
Copy link

Salakar commented Jul 4, 2016

On a side note - it seems these methods are moot for socket connections to non local servers? Using cork / uncork to buffer writes to a local connection practically doubles the op/s. But to a remote server it halves it...? 😭

Is there any different logic for non-local connections? See redis/ioredis#343 that PR I did, I switched to not using cork as it's more performant (~70k ops/s more), but even with a cork/uncork implementation the result on remote servers is the same - worse.

If there is some different logic then is it worth documenting this?

mcollina added a commit to mcollina/node that referenced this issue Feb 7, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: nodejs#7340
@mcollina
Copy link
Member

mcollina commented Feb 9, 2017

Fixed in c38b6d2

@mcollina mcollina closed this as completed Feb 9, 2017
mcollina added a commit that referenced this issue Feb 9, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: #7340
PR-URL: #11222
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 9, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: nodejs#7340
PR-URL: nodejs#11222
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 14, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: nodejs#7340
PR-URL: nodejs#11222
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: nodejs#7340
PR-URL: nodejs#11222
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this issue Mar 7, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: #7340
PR-URL: #11222
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Writable.cork() and Writable.uncork() are meant to be documented
together, but we maintain a lexicographic order. This commit
introduces some links between the two.

Fixes: #7340
PR-URL: #11222
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants