Update Express to v3 + minor enhancements

alabid / flylatex

FlyLatex: A Realtime Collaborative Environment (with a concurrent editor) in node-js.

766 stars 85 forks source link

Update Express to v3 + minor enhancements #48

Closed mkroehnert closed 9 years ago

mkroehnert commented 10 years ago

Main update is about migrating to express v3. Please take a more detailed look at 07d88d3 which might not do exactly the same as before.

Additionally, this update

allows developing on Cloud9 through added host parameter
adds client side form validation of email input
marks all required input fields with the specific html5 attribute
passes the host to the socket.io connection (protocol is still missing)

Next step might be to migrate to express v4.

mkroehnert commented 10 years ago

Just noticed that commit b4d15ab conflicts with #37 which removes the socket.io URL completely.

alabid commented 10 years ago

Hi, thanks for submitting a pull request. It's high that I updated the repo to use express v.3 but I've been quite busy lately. I've looked through most of your changes. It mostly looks good except for one thing. The socketIOURI is undefined because both the socketIOHost and socketIOPort are undefined on the main editor page. Just open the JS console to see the errors. Once the errors appearing on the console are fixed, then I'll happily merge this. Thanks again for your effort. Also, if possible rebase your commits into more atomic commits. The 15 commits you've made can be grouped into < 5. You can use git rebase for this.

mkroehnert commented 9 years ago

I changed the handling of the host/port variables and socketIOURI is now defined again. However, this is still not working correctly when flylatex is served via https.

Regarding the Git commits, I do work with small atomic commits to isolate changes which are independent of each other. Like this you get a commit message for each change and a better log when trying to understand the history of a project. The grouping of commits is then achieved via branches and merge commits.

mkroehnert commented 9 years ago

Closing this one since it seems to have been merged into master manually.

mkroehnert commented 9 years ago

Or maybe not, GitHub seems to show the commits as part of the master repository. However, recent changes made this pull request incompatible and I also switched to using sharelatex.