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

Fix missing Chart library #18

Merged
merged 2 commits into from
Jul 27, 2020
Merged

Fix missing Chart library #18

merged 2 commits into from
Jul 27, 2020

Conversation

trgwii
Copy link
Contributor

@trgwii trgwii commented Jul 25, 2020

ScoreSaberEnhanced currently breaks when pressing "Show pp Graph" on my Firefox 79.0b9 and Greasemonkey 4.9, this PR fixes the issue by including the Chart.js library.

@Splamy
Copy link
Owner

Splamy commented Jul 26, 2020

Do you have an explanation why this happens? Because scoresaber imports its own version of chartjs (<script src="https://scoresaber.com/imports/js/chart.js"></script>) which works just the same.
Merging this probably has no negative effects, I'm just confused
(I'm using Firefox 78.0.2 with Tampermonkey 4.11.6117, so maybe it's greasemonkey doing something different)

@trgwii
Copy link
Contributor Author

trgwii commented Jul 27, 2020

I don't have a solid explanation, my guess is that Greasemonkey probably doesn't use the globals defined by browser scripts, so won't see the scoresaber-imported Chart.js, but for consistency we should probably refer to the scoresaber-imported Chart.js, so I have made the necessary changes for that.

@Splamy Splamy merged commit c23eeaa into Splamy:master Jul 27, 2020
@Splamy
Copy link
Owner

Splamy commented Aug 5, 2020

I now know why. Greasemonkey treats userscripts security wise differently than tamper/violent-monkey in that global scope variables are not passed to the script. This means even if the graphjs lib is loaded in the window, the userscript runs in its own sandbox-like scope which can't see it.
I just pushed a change which removed the graphjs @require since by including it that way it will be run with the initialization of the script (document-start) which is even before the head document node exists. And graphjs tries to add a node to the head which therefore can crash the entire script is some race coditions.
My workaround now is a lazy fetch call which loads the library manually when needed. This should now work for all userscript plugins again.
If this change broke anything again let me know.

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

Successfully merging this pull request may close these issues.

2 participants