Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contribute changes back to original repo #47

Open
rom1504 opened this issue Dec 31, 2023 · 5 comments
Open

Contribute changes back to original repo #47

rom1504 opened this issue Dec 31, 2023 · 5 comments

Comments

@rom1504
Copy link

rom1504 commented Dec 31, 2023

  • half of the components are using react, the other lit
  • prismarine-viewer is hard coded in the repo instead of being a dependency
  • does not use standardjs
  • usage of pnpm
  • half of the code is js, the other half is ts

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
Copy link
Owner

zardoy commented Dec 31, 2023

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
Copy link
Author

rom1504 commented Dec 31, 2023

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
Copy link
Owner

zardoy commented Jan 8, 2024

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

  • mineflayer: Good, waiting for the next release
  • minecraft-protocol: Pick a bunch of untested & p-web-client specific changes: when it was in active development phase I was testing a lot of changes there and I noticed many things need to be reworked just for the web version. There are still many bugs that are related to the web version only (such as the auto version not working 90% of the time). Probably need someone's help with the changes there, not sure.
  • minecraft-inventory-gui a lot of not clean changes "just to make it work", not sure of a clear path here, I'd say it needs rework. I'm planning to bootstrap a few things on top of the current code:
    • jei-like blocks grid on the right - instead of the classic creatives tab (for the first time)
    • better touch support
  • eslint-config-zardoy - awesome extremely powerful eslint config that I really enjoy working with. Literally can't live without it, sorry. I can create a clean version of all rules flattened.
  • diamond-square: commit history. changes seem clean, but the module doesn't get attention.
  • prismarine-world: PrismarineJS/prismarine-world@master...zardoy:prismarine-world:next-era - needs a lot of changes in other modules first
  • prismarine-provider-anvil: seems good for pr: https://github.com/PrismarineJS/prismarine-provider-anvil/compare/master..zardoy:prismarine-provider-anvil:everything
  • zardoy/browserfs I don't have time to investigate the current issues. I'm going to move to fresh browserfs infra when I have time. Since the current build has serious browserfs issues this is one of my prioritized tasks
  • flying-squid ...
  • net-browserify: there is open prs...

@zardoy
Copy link
Owner

zardoy commented Jan 9, 2024

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
Copy link
Owner

zardoy commented Jan 17, 2024

I guess my dream is your nightmare 🤣

image

i have literally everything in this repo

Update: won't be possible to merge.
Discussion pr: PrismarineJS#353
Discord discussion: https://discord.com/channels/413438066984747026/743199746876768318/1249436152226779208

@zardoy zardoy changed the title Cleanup code Contribute changes back to original repo Jun 10, 2024
@zardoy zardoy pinned this issue Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants