search

idkr-client / idkr

:video_game: idk, just a Krunker client
GNU Affero General Public License v3.0
56 stars 37 forks source link

fix userscripts and electron version #36

Closed KraXen72 closed 2 years ago

KraXen72 commented 2 years ago

userscripts were crashing, i fixed it. (the old manager which is still used). also put electron version back to 9.4.4 because thats what idkr 1.2.2 had and it has better performance than electron 12

KraXen72 commented 2 years ago

i reverted the clientUtil change but please check https://github.com/Mixaz017/idkr/pull/36#discussion_r693972438 for my new issue. now the pull request can be merged because it only has the electron version change

KraXen72 commented 2 years ago

any reason why this shouldn't be merged then?

NullDev commented 2 years ago

Ill merge once I'm home to test. Would've probably been better to update with npm i electron@9.4.4 tho since this would update the lock file (package-lock.json) as well.

Mixaz017 commented 2 years ago

Few more things before merging besides the change to the lock file:

The only one breaking change between v9 and v12 that we should care would be the default value for contextIsolation which was changed to true in v12. This value is false by default in v9 which causes errors when contextBridge is used. However this is only used in app/preload/prompt.js, and as far as I'm aware, Krunker doesn't use prompt() anymore (the import button in the settings menu now has its own in-game prompt) but I'm not 100% sure about if its still used somewhere else, especially for non game windows like map editor. If we agree that app/preload/prompt.js should be removed, I will be removing the file and references to the file to reduce the file size. Otherwise we have to explicitly set contextIsolation to true for the prompt window.

My plan is to have master branch with the latest Electron version, and a separate branch with v9. I will create a branch and merge this PR to the new branch if its ok.

Mixaz017 commented 2 years ago

Other than that, v9 seems to work just fine from my testing.

NullDev commented 2 years ago

Sounds good to me @Mixaz017 :+1:

KraXen72 commented 2 years ago

as far as v9 will get all fixes and features from main branch, sounds good 👍 or just like enable contextIsolation lol, isn't it more secure? why would they enable it in v12 if it was a bad thing?

Mixaz017 commented 2 years ago

as far as v9 will get all fixes and features from main branch, sounds good 👍 or just like enable contextIsolation lol, isn't it more secure? why would they enable it in v12 if it was a bad thing?

Our code is already written assuming that contextIsolation is true. What I'm saying is that if we downgrade to v9 and keep the prompt file, the prompt code will break and we must patch it before merging.

KraXen72 commented 2 years ago

okay so propt need to be rewritten right? where exactly is prompt used tho? i thought it was the import settings, but that is now a custom prompt in krunker, so just removing it should be fine?

KraXen72 commented 2 years ago

since you guys said you'd make a branch with v9, i'll just close this pull