Broken live collaboration

[vjeux]

Play by Play

Today at 7:12 AM (9 hours ago) #109 was merged which fixed the build which triggered a new release to be deployed and broke live collaboration.

I tried reverting it but the previous commit actually didn't build. I tried to reverting another pull request ( #112 ) but that didn't build either.

Instead of blindly reverting things, I looked at the commit that last built from heroku (August 21) and reverted the code to that commit ( #113 )

This brought the old version alive and live collaboration is now working again.

Next steps

Right now we should be good with that old version, the problem is mitigated. Some thoughts about what to do next:

• We should figure out how to restore the changes that were done since then.
• We should add a test that makes sure that the build actually works somehow.
• We should have some alarms so we are notified by automation rather than confused customer.

[ahasanen]

Thanks for the quick fix! 🙏

[vjeux]

Looks like alerting would push the price from the current $7 a month to $25 a month.

[asvinours]

first, I'm terribly sorry for breaking the app today.

I tried to reproduce the issue locally by running the frontend and the backend on different domains, and I can't reproduce it.
I'm running the backend repository with the changes from my PR, and all WebSocket requests sent to the backend come back with an access-control-allow-origin header with the value being the domain of the frontend app.
The value of the access-control-allow-origin header on the response is matching the origin header of the request.

I'm down to help fix it but I need to figure out a way to reproduce it first.

[vjeux]

@asvinours no worries, I don't think it was your fault. We shouldn't be able to make the entire server crash by fixing something.

I am not familiar with how to run it or the app itself, @idlewinn, @lipis and @petehunt built it I believe. It would be nice if they could help you out debugging this.

[vjeux]

Numbers are back looking healthy

[dwelle]

Oh, my fault for not properly testing before merging.

Thing is, I'm running the exact same thing on a different server so I assumed it's safe to merge. Nvm, that server isn't actually subject to preflight requests. But I don't see any OPTIONS requests on excalidraw.com either. It's hard to debug this one.

Btw, the handlePreflightRequest is undocumented, and the typings are wrong. There's no Server argument, the signature should be (req: http.IncomingMessage, res: http.ServerResponse). I'm wondering how it could work before at all. EDIT: ok, that seems to be the problem as the latest PR introduced that change to function signature.

[asvinours]

socketio/socket.io-client#1140 (comment)
socketio/socket.io-client#1140 (comment)

I think the second comment is pointing out the issue.
My IDE is telling me the signature of the function is the following: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/engine.io/index.d.ts#L107

[dwelle]

I think the types are simply wrong. The 2.x.x and 3.x.x have the server as thisArg instead of an argument, and in 4.x.x the handlePreflightRequest() API is removed altogether.

[kbariotis]

We should add a test that makes sure that the build actually works somehow.

At the moment, Heroku itself picks up a new commit was merged and starts the deployment process. We can alter that to make a GitHub action do the deploy so we are able to run a test before that that will actually test the server is working before doing the deployment. We also have a staging app on Heroku that we can use to make an integration test but don't quote me on that, it may need a bit more investigation.

There is an issue already #13 that I'm suspecting it was meant to allow doing that but never picked up.

[lipis]

Should be ok now.. no?