search

samuraiexx / roll_together

It's an extension for Google Chrome that synchronizes Crunchyroll Videos that are being played at multiple computers
MIT License
52 stars 12 forks source link

Improve Development Experience + Update Socket.IO Dependency #20

Closed clurdish closed 2 years ago

clurdish commented 2 years ago

There are a few things this PR accomplishes, mostly to make contributions easier and safer.

  1. Updating Socket.IO dependency to match server-side (as of the below PR)
  2. Add source maps for development builds of the plugin
  3. Enable strict mode in TypeScript
    • And fix errors as a result of turning that on
  4. Add types to remote updates to allow TypeScript guards to do their thing
  5. Add window type for background for better type checking across windows
  6. Add env.json file to automatically include environment variables
    • In this case it allowed testing both the server and client locally which just changing a config setting
  7. Conform single quotes to double quotes across TypeScript files
clurdish commented 2 years ago

@samuraiexx @hsnavarro Let me know if you have any feedback on this. Happy to discuss any of the changes.

samuraiexx commented 2 years ago

Looks great! Thank you very much for those changes. I'll also change the server to update the socket.io version and improve the path a little bit (since I need to support old versions I can't just change the server version because of compatibility between socket.io versions). By the way, is there a specific feature of the new version that you think it's worth using? I will wait for @hsnavarro to review before merging but looks amazing!

clurdish commented 2 years ago

I have submitted a PR to the server branch for updating Socket.IO and adding TypeScript support to bring the server in line with the client. See: https://github.com/samuraiexx/roll_together_backend/pull/5

The good news with the server side is that it is backwards compatible with older client versions. So deploying the server update first wouldn't disrupt existing clients, no need to worry about deploying at the exact same time.

In terms of updating to the newer version, I don't see any specific features yet. But I think there are a couple reasons to update

  1. Better TypeScript support. The newer versions include their own type definitions, which are much more reliable
  2. Stay up to date with an actively maintained major version
clurdish commented 2 years ago

@hsnavarro Any feedback on this from your end?

hsnavarro commented 2 years ago

@hsnavarro Any feedback on this from your end?

Hey, I already told @samuraiexx to approve it, but he probably forgot. I'll remind him again.