webxdc / webxdc-dev

A development server for webxdc apps
The Unlicense
20 stars 4 forks source link

Delete devices #29

Closed Septias closed 2 years ago

Septias commented 2 years ago

This pr adds a delete button to each device so devices no longer used can be trashed

Septias commented 2 years ago

Hey @faassen are you still actively developing this project? I've seen there has been no activity on my other pr as well for a long time. If you however find the time maybe you can look into this because I know too little about websockets & express and how to idiomatically shut down a ws-instance. As you can see in this pr, I deleted all references but the ws still seems to live on until a new requests comes in and the ws crashes because important stuff has been deleted. This however leads to full termination of the virtual device which is what we want in the end, but it also logs an error in the console which is not so nice and I would like to fix that.

faassen commented 2 years ago

I'm not very actively working on this right now, but if you ping me here or on deltachat to remind me I can take a look once every while.

Concerning deleting devices: I'd be interested in hearing more about the use cases surrounding this - I guess it simulates a group member being removed from a group? Because intermittent connections can already be handled by disconnecting.

As a general comment, message.test.ts contains a set of tests for the underlying model. Your modifications should come with new tests that exercise the delete case.

I left some comments on your code. I am still trying to understand the behavior you describe.

faassen commented 2 years ago

Oh, another small note, in your codebase "delete" comes before "add" in instance.ts which is a bit surprising if you read it.

faassen commented 2 years ago

As I mentioned, using delete to wipe properties off instances isn't the way to go.

I don't really know how to close an express server either so I googled and found this:

https://github.com/expressjs/express/issues/1366#issuecomment-68693264

So if in instance.start you keep a reference to this on the instance, you may be able to use it to close it later. Though I think given that you send a 'delete' message from the backend you'd want to do this after this message was sent somehow.

Maybe that helps!

Septias commented 2 years ago

Thank you for this very quick response and I'm sorry you had to make so many simple corrections. This PR isn't really ready for review yet, I just pinged you because I wanted to know how to correctly shutdown ws instances. I also pushed unfinished changes in start implementing delete Listener for you to see that I actually have some idea on how to solve the ws shutdown problem but the commit wasn't really cleaned by me, so again, sorry you had to do so much tedious reviewing. I'll change every mentioned point and work things out before requesting review again

Septias commented 2 years ago

Regarding the usecase of this, I took it out of the todos when I was looking for stuff that could be done. Removing an instance which is no longer needed after some expressive testing seems like a reasonable use case - though it is also not a must have. What do you think? As for what this means in webxdc space, removing the deleted device from the group with disconnect seems neccessary so I will probably implement that aswell.

Septias commented 2 years ago

I've looked into the disconnect behaviour and deleting a device is essentially a disconnect which is already covered by tests. In the end deleting a device is just a glorified disconnect so I did not add more tests