zardoy / minecraft-web-client

The most advanced Minecraft online/offline client + server in your browser! [IOS SUPPORTED]
https://mcraft.fun
MIT License
30 stars 30 forks source link

Contribute changes back to original repo #47

Open rom1504 opened 8 months ago

rom1504 commented 8 months ago

are you interested for fixes for some of these ?

I figure we might as well fix your fork, and when it becomes ok enough, we merge back to upstream. What do you think?

zardoy commented 8 months ago

Hey! First of all thank you so much for looking into my code! Really damn appreciate it!

I figure we might as well fix your fork

I will try to help with anything needed if you're interested in this!

if you wonder what was my intention to rethink & improve the whole infra from the ground: I forked it at the end of the summer because I got a side request to make the application load worlds in fully offline mode + p2p multiplayer, I didn't have much time for testing and I had to dramatically speed up development, and I instantly realized what slows me down and made everything tested. This was done just for developer productivity! Generally, if some other guy doesn't know how to work with that he shouldn't be afraid of it :)

does not use standardjs

There are no TS repos using standardjs and this repo has much much more powerful linting setup. Yes, I'm thinking out of PrismarineJS repos scope, sorry.

usage of pnpm

Anything bad? I enjoy fast & efficient installs.

prismarine-viewer is hard coded in the repo instead of being a dependency

I don't mind extracting it back to a dependency. Initially, I wanted to turn this repo into monorepo since p-viewer is a core of the whole client, most of the time I needed to fix something in p-viewer for the client so I can release the changes instantly, but I don't mind putting it back.

half of the code is js, the other half is ts

What do you want? Just remove the large piece of work working on the types system (remove all the types for no reason)? we can also can put types into js with less concise syntax but I don't see a reason to do this, on the other hand this is better than nothing and this is what bluemap repo did

I'm not the only guy using all these in a public github repo, for example famous DefinitelyType is also using pnpm...

Again, thank you so much for your time and for keeping your interest up!

rom1504 commented 8 months ago

Ideally I would like to keep coherence with the rest of PrismarineJS, but if not, at minimum I would like to have internal consistency in this repo.

So for example if we're using typescript use it for all files.

If using react, let's switch fully to it.

But I'll play with some of these tools and come back to you, that seems to be an easier path than arguing over it.

zardoy commented 8 months ago

Move from my dependencies (and see what is blocked):

zardoy commented 8 months ago

about pnpm: we can remove this large lockfile. I'd be more than happy with the solution of keeping the repo without lockfile, but we will have to lock all github deps to specific commit hash instead of branch in package.json for reproducible builds and use this script if want to update all deps: https://github.com/zardoy/prismarine-web-client/blob/aea90db31e398682de6aca827c162253e9b4ba5f/scripts/updateGitPackages.mjs#L5, but I'd prefer keeping pnpm on the ci to ensure we have the fastest builds. Also I'd prefer keeping vercel, can live without these awesome preview deploys that stay forever :)

zardoy commented 8 months ago

I guess my dream is your nightmare 🤣

image

i have literally everything in this repo

Update: won't be possible to merge. Discussion pr: https://github.com/PrismarineJS/prismarine-web-client/pull/353 Discord discussion: https://discord.com/channels/413438066984747026/743199746876768318/1249436152226779208