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

Should show error screen on error #350

Merged
merged 5 commits into from
Sep 16, 2023
Merged

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Aug 18, 2023

With this, it should show the error screen when an error actually happens (right now setTimeout seems to be removing it).

Though in my fork I've rewritten the error screen so it's now possible to go back without having to reload the page. But I've also rewritten the screen history management, so I don't think I'll ever have time to cherry-pick so many commits, so I'm providing a simpler copy-paste solution for now.

@zardoy
Copy link
Contributor Author

zardoy commented Aug 18, 2023

Let me know what changes you will be interested in. I have changed the directory structure but I think I can still try to copy-paste some improvements & important bug fixes (such as incorrect creative bars update) ... or copy all changes along with reverting some of the commits

@rom1504
Copy link
Member

rom1504 commented Aug 25, 2023

please fix the ci

@zardoy
Copy link
Contributor Author

zardoy commented Aug 26, 2023

I don't think changes in this PR caused ci failure. I had the same problem with Webpack (and Vite) locally. Seems they work badly when it comes to bundling large files (like these json files). CopyPlugin also may cause it I believe. I have solved these problems in one of the first commits, so have to remember.

Anyway, I'll try to adjust the build process in this PR then. if you are interested in some other changes please let me know, I will try to rebase and group changes you interested in.

Update: FYI not sure why (it doesn't happen on ci & in docker) but when I try to run npm i locally I get ERESOLVE in rollup dep if I don't up deps manually.

@rom1504
Copy link
Member

rom1504 commented Aug 26, 2023

indeed CI is broken in main https://github.com/PrismarineJS/prismarine-web-client/actions/runs/5984401321/job/16235532976?pr=352

it needs to be fixed before we can merge anything

@zardoy
Copy link
Contributor Author

zardoy commented Aug 26, 2023

I have solved these problems in one of the first commits, so have to remembe

Sorry, I lied, actually I just removed bedrock files from the build completely (because they have too large blocksj2b files). However, bedrock is supported here, right? Do you know how blocksj2b are used here or we can remove them? I'm not even sure how these files are used in prismarine projects at all yet.

@rom1504 rom1504 merged commit 2fba863 into PrismarineJS:master Sep 16, 2023
3 checks passed
@zardoy zardoy deleted the error-handle branch September 19, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants