Skip to content

use process.stdout.write on server instead of fs.write #5408

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

Merged
merged 2 commits into from
Oct 27, 2015

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Oct 26, 2015

fixes VSCode regression introduced in #5354.
pinging @zhengbli, can you check if this solution still solves #2758 and microsoft/TypeScript-Sublime-Plugin#379

}
else {
canWrite = false;
process.stdout.write(new Buffer(s, "utf8"), () => {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to create a new buffer each time or can you avoid allocating one per call?

Copy link
Member

Choose a reason for hiding this comment

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

Consider also moving the callback out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the common practice and standard buffer API. In theory we can pool then though in practice I'll start doing this only if I have convincing numbers

@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2015

👍

@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2015

we will need to port this to release-1.7

@zhengbli
Copy link
Contributor

Just curious, what is the VSCode regression?

@vladima
Copy link
Contributor Author

vladima commented Oct 27, 2015

@zhengbli code in VSCode that read data from the stdout of the tsserver process stopped working after tsserver was switched to the asynchronous writing. The same code worked perfectly fine with the old implementation and with the proposed fix - tested on Windows and Linux machines

@zhengbli
Copy link
Contributor

@vladima got it. Thanks for the explanation! Though I need to test the fix on a OSX machine, which I don't have at hand. I'll test it when I get back to office tomorrow evening.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 27, 2015

let's get this in first as this is blocking VSCode, and then we can update the fix if the original issue was not fixed.

@zhengbli
Copy link
Contributor

reasonable. If this fix works for vscode on windows, mac, and Linux then 👍

vladima added a commit that referenced this pull request Oct 27, 2015
use process.stdout.write on server instead of fs.write
@vladima vladima merged commit 847a074 into master Oct 27, 2015
@vladima vladima deleted the asyncWriteOnServer branch October 27, 2015 16:59
@zhengbli
Copy link
Contributor

Confirmed the fix works on both Mac and Windows for sublime text.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants