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

Capture data between terminal creation and connected to terminal in demo #4194

Merged
merged 4 commits into from
Oct 15, 2022

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Oct 9, 2022

When I opened the demo terminal, the first line was missing. The terminal is functioning whatsoever. Looks like the terminal first line is sent before the terminal frontend is connected. Thanks to ws.send(logs[term.pid]); that line is sent to the terminal frontend when the terminal gets connected.

This commit introducing this: 193d305
This reverts it but keeps the higher timeout.

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.

Thanks! I saw this but had no idea why it was happening.

demo/server.js Outdated
Comment on lines 58 to 61
logs[term.pid] = '';
term.on('data', function(data) {
logs[term.pid] += data;
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we dispose of this listener once the terminal is connected? So after right that ws.send call? That will give us the best of both worlds.

We could then rename logs to something more descriptive to prevent this accident in the future, like unsentOutput?

Copy link
Member

Choose a reason for hiding this comment

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

Oh well - yeah I removed it because it is a nuisance during perf measuring (backend constantly dies after testing a serious amount of data) - and I wasnt aware of the race condition (never happened for me). I think removing the aggregation buffer once the connection was successful would give us both advantages.

@Tyriar Tyriar added this to the 5.1.0 milestone Oct 9, 2022
@silamon silamon changed the title Adds logs again in server.js Capture data between terminal creation and connected to terminal in demo Oct 15, 2022
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.

Thanks 😃

@Tyriar Tyriar merged commit 869ce3a into xtermjs:master Oct 15, 2022
@silamon silamon deleted the fixmissingline branch October 16, 2022 09:52
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.

3 participants