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

write callback support #2422

Merged
merged 19 commits into from
Sep 26, 2019
Merged

write callback support #2422

merged 19 commits into from
Sep 26, 2019

Conversation

jerch
Copy link
Member

@jerch jerch commented Sep 12, 2019

PR for callback support in write methods. Also simplifies the write interface supporting both string and raw UTF8 bytes (Uint8Array) in one method.

With the changes its possible to get notified once a chunk of data was parsed by xterm.js:

  • with string data
...
await new Promise(resolve => term.write('some string data', resolve));
doSomething(); // executed after the chunk was processed
...
  • with utf8 bytes
...
await new Promise(resolve => term.write(new Uint8Array([66, 67, 68]), resolve));
doSomething(); // executed after the chunk was processed
...

Fixes #2408.

@jerch jerch added this to the 4.1.0 milestone Sep 12, 2019
@jerch
Copy link
Member Author

jerch commented Sep 12, 2019

@Tyriar Was worried that merging the input types along with the additional callback handling would create a negative perf impact, but thats not the case:

   Context "out-test/benchmark/test/benchmark/Terminal.benchmark.js"
      Context "Terminal: ls -lR /usr/lib"
         Context "write"
            Case "#1" : 5 runs - average throughput: 29.05 MB/s
         Context "writeUtf8"
            Case "#1" : 5 runs - average throughput: 35.94 MB/s

Thats on par with our current separated handling without any callback stuff. Same results in the demo with ls -lR /usr - no significant difference.

addons/xterm-addon-search/src/SearchAddon.api.ts Outdated Show resolved Hide resolved
addons/xterm-addon-webgl/src/WebglRenderer.api.ts Outdated Show resolved Hide resolved
src/Terminal.test.ts Show resolved Hide resolved
src/Terminal.ts Outdated Show resolved Hide resolved
src/public/Terminal.ts Show resolved Hide resolved
src/public/Terminal.ts Outdated Show resolved Hide resolved
test/benchmark/Terminal.benchmark.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks good so far, still a todo list to action before merging

src/Terminal.ts Show resolved Hide resolved
@Tyriar
Copy link
Member

Tyriar commented Sep 23, 2019

@jerch is this ready for review/merge?

@jerch
Copy link
Member Author

jerch commented Sep 23, 2019

@Tyriar Yepp I think so, well - might need some polishing.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

lgtm, nice work! 👍

@Tyriar Tyriar merged commit ea922fe into xtermjs:master Sep 26, 2019
@Tyriar
Copy link
Member

Tyriar commented Sep 26, 2019

🎉

microsoft/vscode@c2e7944

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.

Unable to get value synchronously
2 participants