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

Pick all new changes #353

Closed
wants to merge 334 commits into from
Closed

Pick all new changes #353

wants to merge 334 commits into from

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Sep 19, 2023

Preview deploy: https://mcraft.fun or https://mcon.vercel.app - PCM.GG for short.
Hey! This PR adds all features from my fork without any exceptions. I rebased this version using my script so it doesn't introduce the viewer into the repo. Yes, I know there are a lot of commits, so I think I'll rebase it one or more times to squash some commits, but it already can give you a final vision of the project. Maybe I'll think of re-grouping some commits (and open some separate prs if you ask). There is a diff with my main branch. Again, the reason I'm opening this pr is to have some kind of initial feedback so I can better understand what things you need, what things you don't like, etc...
There are some notable changes:

  • migrated to esbuild. this is not because webpack was too slow, but because it was giving a lot of unpredictable behaviors. I spend countless hours debugging webpack dev & prod configs, esbuild config is just easier to maintain for me as there is no dev and prod modes. I'm not going back to chaos, see Make Webpack faster #210 (comment)
  • fully migrated to ts
  • react. didn't push these changes yet, but i have fully migrated from lit. the only reason for this is shadowdom. imo react easier to maintain at scale with its great ecosystem. I'm going to setup storybook for components soon.
  • on CI there are vercel deploy workflow and ci.yml with cypress testing (still working on some tests). But one integration test (singleplayer testing) already can cover a bunch of possible issues

fixes #10
fixes #33
fixes #47
fixes #117
fixes #210
fixes #253
fixes #241
fixes #274
fixes #287
fixes #304
fixes #302
fixes #66 (if you going to setup vercel project)
and many others not reported here...

general tasks:

  • remove my deps (12) (not possible currently)
  • remove vite and webpack configs & deps as they are not used anymore
  • Remove my links
  • Restore linting (eslint)
  • fix: no disconnect on: 1) wrong version specified 2) too long username 3) disconnect button click (but still sometimes no pong issues)

1. watchable for changes
2. can be used along with react
…l+w), but not when paused so can easily close if desired
add mobile button
review upstream
wip react refresh (thats why migrating to react)
to migrate to tsx
to impl advanced settings
…adding flying

add settings like the one to disable in-game auto-update
inspect dragndroped nbt data!
improve disconnect action (disconnect immediately)
TODO not pixel-perfect
@socket-security
Copy link

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue Package Version Note Source
Potential typo squat eslint-config-zardoy 0.2.17

Next steps

What is a potential typo squat?

Package name is similar to other popular packages and may not be the package you want.

Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore eslint-config-zardoy@0.2.17

@zardoy
Copy link
Contributor Author

zardoy commented Sep 27, 2023

I expect you to handle config/workflow files as you want. Feel free to guide me in the right direction. I'll stop adding new commits here as there is already extremely big.

@rom1504
Copy link
Member

rom1504 commented Nov 4, 2023

CI is failing. Can you fix ?

@@ -0,0 +1,89 @@
{
"extends": "zardoy",
"ignorePatterns": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this file ? We use standard

uses: actions/checkout@master
- name: Install pnpm
run: npm i -g pnpm
- run: pnpm install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why pnpm ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of pnpm lock i guess

Copy link
Contributor Author

@zardoy zardoy Nov 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should not matter what pm you use, but I had to put lockfile into the repo so I can lock deps for fully reproducible builds (without sacrificing branch information stored in package.json). So, yes, you are forced to use pnpm for reproducible builds, but if you don't care you actually use any other pm.
i personally use pnpm because obviously, it is fast for external deps (gh, npm) (when I started working on all this my initial goal was to make ci as fast as possible). I hope it answers your question, but since it's not my project I don't mind any changes in workflow files (you decide)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Npm also supports package lock. Doesn't it ?

@@ -0,0 +1,61 @@
{
"configurations": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? how do you debug this project otherwise?

@@ -0,0 +1,79 @@
/// <reference types="cypress" />
import type { AppOptions } from '../../src/optionsStorage'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there typescript code here ?

@@ -0,0 +1,26 @@
//@ts-check
import mcServer from 'flying-squid'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why .mjs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why .mjs ?

Because I like import syntax highlight.
I generally more like esm because of top level await.
And yes I'm aware of esm restrictions (__surname and so on

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use .mjs anywhere else in PrismarineJS. Syntax highlighting works there though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok if you want me to remove it won't be a problem. You just asked why…
And no, syntax highlighting doesn't work in cjs files in the same way as it works in esm (I mean import keyword!)

"dependencies": {
"@dimaka/interface": "0.0.3-alpha.0",
"@types/react": "^18.2.20",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why react ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the type of guy who just decided to make a full rewrite using my favorite frameworks. I removed lit in the latest version not only because React has a much greater ecosystem (such as good storybook support) & clean function components and I didn't see a single reason to use lit. I could adopt, really, but there was a thing that was blocking a lot of stuff. The main reason why I had to drop lit is the shadow dom (e.g. no way of using global CSS variables).
I've enjoyed the rewrite and am happy with the final result, but if you're not I'm sorry! I actually didn't see other solutions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no way of using global CSS variables

this is one of the best feature of lit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one of the best feature of lit

How?

This is the same as saying that "no way to build scalable applications is also the best feature of lit". These kinds of restrictions don't make code more maintainable it just makes it harder to find solutions. Meanwhile react has the best ecosystem you can find if you're a frontend developer, it still is the fastest & easiest way to build robust UI at scale. Just take the current storybook integration: zero-config first-class react support. I tried to adopt lit, but it sucks at absolutely everything, no good state management, no hmr, plugins and so on... Also, my team could easily work with react + ts and even build new components without any problems so I don't see objective points here, for most other devs it doesn't really make the difference what framework you use...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global css makes it impossible to build reusable components, it's not much more complicated than that

that's why react doesn't really have reusable components but instead it has sets of components you need to all use together

beyond that, regarding what framework to use here, I am not yet convinced we need anything big as react / state management / etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the only way forward here is for me to go and pick stuff that makes sense from your fork, and eventually we converge to something reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But React is not that big and at some points even easier to learn. You just need to understand how Function components work and that's. And what is more reasonable from your point? This is an absolutely fantastic & simple UI framework (any frontend guy would say you the same), I don't understand what's the issue with that (sorry)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I'm the frontend guy and React is pretty bad, actually. There's much simpler ones like Svelte, and more performant ones like Solid. This project probably uses Lit for a good reason, like reducing bundle size, and we need to respect that (unless majority agrees to migrate to something else).

interactPlace: [null, 'Left Trigger'],
chat: [['KeyT', 'Enter'], null],
command: ['Slash', null],
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in typescript?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an enforcement to not use it, it won't be a problem for me to not add the blue bar to your repo. I added it for my personal needs (to speed up development & gain confidence and for some other contribs as well). I should not care about your infra decisions, but it would be great if you could participate and say what you want to be picked. I will also try to get another dev that could probably adopt the infra changes.
That's also the reason why I threw away the idea of rebasing & splitting it into smaller (a bit) prs. I just need to understand what is allowed here and what is not. Again, some kind of task list would be an ideal solution. You can hop to my repo if you want some kind of build preview. Also, please feel free to ask anything, I can explain anything you want.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could barely read more than 500 commits (I believe the commit history could be much cleaner) and I noticed a huge number of various important fixes so I would not call this just a small bag of improvements. That’s why I really I wish it could be available some day (really want to try it out), so @zardoy why did you have to do this unnecessary infra changes, why you couldn’t skip this part!? Also, doesn’t make ts harder to contribute as you need to learn & write types?

@rom1504
Copy link
Member

rom1504 commented Nov 4, 2023

Too bad I didn't have time to review before but yeah in the current state we can't merge.

This is a mixed bag of good improvements and unnecessary infra changes.
Idk what to do with it.

If you were to propose each large change independently it would be possible to discuss each change and eventually reach something we can merge.

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

tried https://mcraft.fun/ today

I got to admit the single player mode is pretty cool

let's see how we can get this merged

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

can you think of some ways to first include functional changes here with no refactoring ?

or do technical PRs with no functional changes ?

@Saiv46
Copy link
Contributor

Saiv46 commented Jan 8, 2024

I was about to start fixing #210 by migrating to esbuild, but holy shit this is a huge PR.

This will probably not gonna to be merged anytime soon in one piece. Can we try to split it into smaller ones?

@Saiv46
Copy link
Contributor

Saiv46 commented Jan 8, 2024

I'll leave it here: zardoy#1 (fork's roadmap, includes fixes)

@zardoy
Copy link
Contributor Author

zardoy commented Jan 8, 2024

@Saiv46 I understand that and I went tooo far with this one. Even this pr is just extremely outdated (tho I didn't do esbuild pipeline changes much recently), and I wish I had more time & energy to split to more prs here. I was too interested in implementing anything and couldn't focus on small prs much at that time (not great I know).

Can we try to split it into smaller ones?

I literally wanted to hire someone to help me with this one, if you could collaborate it would be really really good. As was said in zardoy#47 some things probably would be better to pick from the main branch (if you are interested), let me know where we can start

I'll leave it here: zardoy#1 (fork's roadmap, includes fixes)

hm, what do you mean?

@zardoy
Copy link
Contributor Author

zardoy commented Jan 8, 2024

but holy shit this is a huge PR

btw as I remember I needed one hour to just read the code changes, but esbuild pipeline changes are not that big

@rom1504
Copy link
Member

rom1504 commented Jun 9, 2024

To clarify here, this cannot be merged as is.

You have 2 ways forward

  1. Introduce changes one at a time to PrismarineJS
  2. Or simply rename your repo to ZaydoyGUI or whatever name you like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
4 participants