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

cork/uncork logic client fix #147

Merged
merged 2 commits into from
Mar 26, 2019
Merged

cork/uncork logic client fix #147

merged 2 commits into from
Mar 26, 2019

Conversation

scarry1992
Copy link
Contributor

No description provided.

stream.js Outdated Show resolved Hide resolved
stream.js Outdated
if (isBrowser) {
stream = proxy
stream.cork()
socket.onopen = onopenBrowser
Copy link
Collaborator

Choose a reason for hiding this comment

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

can the same logic be applied to Node as well? What will cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to use same logic for Node and browser, but for Node needed duplexify logic for destroy() method as I see.

@scarry1992
Copy link
Contributor Author

scarry1992 commented Jan 28, 2019

@mcollina, if structure fixed. So, what do you think?

@mcollina
Copy link
Collaborator

I don't really understand why we can't use the same logic for Node as well.

@scarry1992
Copy link
Contributor Author

scarry1992 commented Jan 28, 2019

@mcollina, You can try to clone and remove last else block from upper code block and to run server tests. As I see lib use some destroy logic included in duplexify for server side using lib. Maybe I understood something wrong.

@scarry1992
Copy link
Contributor Author

@mcollina, Hello. Do you tried to rewrite w/o duplexify?

@mcollina
Copy link
Collaborator

mcollina commented Jan 29, 2019 via email

@scarry1992
Copy link
Contributor Author

@mcollina, so what about that PR?

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM, after coming back to this, this seems ok.

@mcollina mcollina merged commit 441a94a into max-mapper:master Mar 26, 2019
@scarry1992
Copy link
Contributor Author

@mcollina, do you release it in npm?

@mcollina
Copy link
Collaborator

Yes, v5.2.0.

mcollina added a commit that referenced this pull request Mar 27, 2019
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