Skip to content
This repository has been archived by the owner on Feb 13, 2023. It is now read-only.

fix userscripts and electron version #36

Closed
wants to merge 6 commits into from

Conversation

KraXen72
Copy link

No description provided.

@KraXen72
Copy link
Author

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

@CreepSore CreepSore requested a review from NullDev August 21, 2021 21:38
@CreepSore CreepSore assigned CreepSore and unassigned CreepSore Aug 23, 2021
@KraXen72
Copy link
Author

i reverted the clientUtil change but please check #36 (comment) for my new issue. now the pull request can be merged because it only has the electron version change

@KraXen72
Copy link
Author

any reason why this shouldn't be merged then?

@NullDev
Copy link
Member

NullDev commented Aug 23, 2021

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

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

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

@NullDev
Copy link
Member

NullDev commented Aug 24, 2021

Sounds good to me @Mixaz017 👍

@KraXen72
Copy link
Author

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

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

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?

- yeeted menutimer

should be up to date with mixaz's master

Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes are pretty much unnecessary (or at least needs to be polished)

Copy link
Author

Choose a reason for hiding this comment

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

sorry my bad i wasnt aware these go to this pull request, i kinda just made my own fork lol


#package lock, will be generated anyway on install
package-lock.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lock file should not be ignored

Copy link
Author

Choose a reason for hiding this comment

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

sory i wasn't aware my commits went to this pull, but like why should it not be ignored? it gets generated from npm anyway, so having it there has no effect

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json
Maybe try searching before asking next time

@KraXen72
Copy link
Author

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

@KraXen72 KraXen72 closed this Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants