chore(site): remove terminal xservice #10234
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e0b78cc to
504c797
Compare
504c797 to
8041609
Compare
code-asher
commented
Oct 19, 2023
To make it easier to move away from this service.
To make it easier to move away from this service.
This is a prelude to the change I actually want to make, which is to send the size of the terminal on the web socket URL after we do a fit. I have found xstate so confusing that it was easier to just rewrite it.
I am not really sure what ws.connected is doing but it seems to somehow block updates. Something to do with `act()` maybe? Basically, the useEffect creating the terminal never updates once the config query finishes, so the web socket is never created, and the test hangs forever. It might have been working before only because the web socket was created using xstate rather than useEffect and once it connected it would unblock and React could update again but this is just a guess.
The terminal only cares about the renderer specifically, no need to recreate the terminal if something else changes.
8041609 to
da4a231
Compare
Felt like this could be broken out to reduce the component size. Also trying to figure out why it is causing the terminal to create multiple times.
Depending on the timing, handleWebLink was causing the terminal to get recreated. We only need to create the terminal once unless the render type changes. Recreating the terminal was also recreating the web socket pointlessly.
da4a231 to
15d3f58
Compare
BrunoQuaresma
approved these changes
Oct 20, 2023
|
Related to #9942 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a prelude to the change I actually want to make, which is to wait for a
fitbefore connecting the web socket so we can send along the height/width and initialize screen with the right size.I was having trouble modifying the xservice and it was pretty easy to simply remove it so I opted for the latter since we have been removing them anyway.
Let me know if this seems like a bad idea though.
I did not have such an easy time with the tests though. For reasons I do not understand, calling
connectedon a web socket created byjest-websocket-mockappears to freeze all updates to the component. I usednextMessageinstead since there is always an init message we can use to detect when the socket is connected so it should be fine, but it is still unfortunate.Part 1 of fixing #9806
Edit: moving to draft while I look into why it sometimes creates the terminal twice.might have been an existing problem but it is fixed now