sizzlemctwizzle / GM_config

A lightweight, reusable, cross-browser graphical settings framework for inclusion in user scripts.
https://github.com/sizzlemctwizzle/GM_config/wiki
GNU Lesser General Public License v3.0
205 stars 48 forks source link

Calling get() before init() completes doesn't work anymore #113

Open sizzlemctwizzle opened 1 year ago

sizzlemctwizzle commented 1 year ago

This is a known and unavoidable breaking change that will require migration to the new usage format. This issue is the direct result of fixing #92. Please read the wiki page before commenting on this issue. It provides examples of three different ways to adapt affected code.

The purpose of this issue is to provide greater visibility for those who are impacted by this breaking change. Any reported issue that is due to this same change will be closed and marked as duplicate. This issue will be closed, along with the milestone, after a minimum of one year has passed (May 20th, 2024), but documentation in the wiki will be retained indefinitely.

This issue is also a place to ask questions regarding the full impact of the move to asynchronous read/write: a discussion that can ultimately improve the documentation.

Edit: No legacy version will be provided. I'm not going to maintain two versions of GM_config. Anyone can fork this library and use git reset 2207c5c --hard, or on OUJS with this version.

sizzlemctwizzle commented 1 year ago

@Martii Just waiting on the angry mob of script authors whose scripts I just broke in a difficult way to detect and identify. This is one of the reasons I've avoided making breaking changes up until this point. It is difficult to get the word out. Hopefully 340d138 helps. I've done all that I can for now.

frank-trampe commented 1 year ago

I'm not sure how many people whose scripts broke are going to comment, but I will say that I'm probably not the only one who has a lot of synchronous functions in a lot of scripts that call the get method. I am probably also not the heavy GM_config user who does not follow developments on the issue tracker. Given that the change breaks the most-called method in the library in probably the majority of its use cases in a way that requires significant rewriting to accommodate, I should have expected that you would provide a quick work-around in the error message, like a hosted copy of the legacy code. Instead, I found myself scrambling to paste the old version of your code into a number of scripts. I plan to test the legacy version hosted here soon.

Martii commented 1 year ago

@frank-trampe

Given that the change breaks the most-called method in the library in probably the majority of its use cases in a way that requires significant rewriting to accommodate, I should have expected that you would provide a quick work-around in the error message

Did you read the url that was referenced in the console after you did a basic self retest with a reinstall of your .user.js? (also take a look at the top of this issue for the wiki article)

As a Collaborator I too had to do a very minimal rewrite (twice to provide an alternate that I've previously linked in here). While I can't speak to every code, most code I've seen only takes at minimum a 3 line change (5 if you want GM4 proper native compatibility) and that is with code that is already asynchronous or synchronous overall.

sizzlemctwizzle commented 1 year ago

I plan to test the legacy version hosted here soon.

That is not a legacy version but actually a fork. The two versions are incompatible, but share a common ancestor. The first version of GM_config was written by myself and the author of that fork in '09.

who does not follow developments on the issue tracker

expected that you would provide a quick work-around in the error message

The error message did its job by bringing you to this issue where you could be properly informed. You didn't need to "follow developments".

breaks the most-called method in the library in probably the majority of its use cases

I humbly apologize to the edge-case script author who actually has to do a lot of work to accommodate this change. Most use cases require simple re-working for pre-init get calls (see the wiki). Anyway, I apologize to everyone for the hassle.

Martii commented 1 year ago

@sizzlemctwizzle

Now that I'm back at dev station I'm going to spend some time fixing his fork database entry to reflect that... redone (and fixed it for his other library too)... EDIT: a url has changed but not his GM_config , using the libs route for that other lib, EDIT: Modified OUJS for this special use case of his GM_config. Manual setting btw.

reyaz006 commented 1 year ago

Yep, no idea what this is all about and too busy to spend time on re-designing scripts. Maybe just provide the latest version that "works fine" and let authors know they could switch to that?

Martii commented 1 year ago

@reyaz006

For your fork that I've never seen and have no clue what the script does it seems that the change should be:

--- /scripts/Marti/MetaBot_for_YouTube/source@230102
+++ /scripts/Marti/MetaBot_for_YouTube/source
@@ -111,6 +111,7 @@
 'default': '51328'
 }
 },
+init: onInit,
 });

 const checkb = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABYAAAAPCAMAAADXs89aAAAA+VBMVEUAAAD///+qqqp/f39mZmZubm5vb29sbGxtbW1tbW1tbW1ubm5ubm5tbW1ubm5tbW1ubm5ubm5ubm5tbW1tbW1ubm5ubm5tbW1ubm5ubm5tbW1ubm5ubm5vb29wcHBxcXFzc3N0dHR1dXV3d3d4eHh6enp7e3t8fHyAgICCgoKFhYWMjIyOjo6Pj4+QkJCSkpKUlJSWlpaZmZmampqdnZ2hoaGqqqqwsLC0tLS1tbW2tra5ubm+vr7ExMTKysrLy8vQ0NDR0dHS0tLU1NTV1dXW1tbe3t7i4uLj4+Pk5OTl5eXn5+fo6Ojq6urs7Ozu7u7w8PD9/f3////SCMufAAAAHHRSTlMAAAMEBSUnKCpbXV9htre6u87Q0dPp6uvs7u/8pkhKVQAAAMdJREFUeNpN0NdWwlAUANEbgvTeQhnQIE1B1ChYQcEC0gL+/8ewzPWwmMf9OMowDKWUP5YqU0pGTaXTHMqdOcM7G7LBIw4Vnc3b89i9BytwYH+uv7oAOqsbyJjCMb4d/urOgYhwqr55wmvdhIRwufXT0zzrgCVM1X2tA5xva1ARLvE4aQCMXoCCcJLaZNpvX71/3QJx4ShUBx+Lz4fp7hrCwmYWL3v5u7XTPmEVtLRfuk7+5OhJIKP9NO2psDIjCatSiId9/7oHY28awgWqV+8AAAAASUVORK5CYII=';
@@ -156,6 +157,7 @@
 var txtlistpadd = '\u2003<span id="listpadd" style="cursor: pointer; ' + iconstyledef + '" title="Добавить в закладки">' + iconsdef[0] + '</span>';

 console.log("[MetaBot for Youtube] Starting at URL: " + window.location);
+function onInit() {
 if (window.location.hostname == "dislikemeter.com" || window.location.hostname == "www.dislikemeter.com") {
 var videoid = getURLParameter('v', location.search);
 if (videoid) {
@@ -178,6 +180,7 @@
 init();
 }
 }
+}

 function init() {
 if (document.head.innerHTML.indexOf('window.ShadyDOM') >= 0) {
@@ -1160,4 +1163,4 @@
 }
 }
 waitForKeyElements.controlObj = controlObj;
-}
+}
\ No newline at end of file

... estimated change time less than 60 seconds. Estimate skim time 3 minutes (my learning curve approximation for your forked code which you probably know way more about it). Do you think you can beat that time in your "busy" schedule? Took me more time to create this post as I suspect you did the same.

Point of the matter is the migration is necessary and if you use an older version it will have issues that are not fixed. I, and my network, am thrilled that this change increases the scope of GMC even more.

sizzlemctwizzle commented 1 year ago

no idea what this is all about

It's about this.

reyaz006 commented 1 year ago

@Martii thanks, I may try that at some point. Though it seems awkward to be required to move the whole code into a 3rd party function, but alright.

It's about this.

Yeah, I mean no information provided about why this was needed in the first place and why no backwards compatibility was provided. E.g. why not create GM_config2 instead etc.

frank-trampe commented 1 year ago

The title of this issue is misleading if not wrong. get() is still synchronous and still works properly as long as it is called after the first invocation of init() completes/resolves. I did not bother to read the wiki page until after I looked at the code because I figured that it would just tell one to make every function async. Upon realizing the specific issue, I just captured the promise from GM_config.init() (previously discarded) and chained the on-load functionality for my script off of that promise. Tests pass, and I am going to start applying the change to other scripts. I would suggest changing the issue title from "Synchronous usage of get() doesn't work anymore" to "Calling get() before init() completes doesn't work anymore" or something like that.

sizzlemctwizzle commented 1 year ago

@frank-trampe

I would suggest changing the issue title

Done. Thanks for the suggestion.

I just captured the promise from GM_config.init() (previously discarded) and chained the on-load functionality for my script off of that promise.

Could you please provide an example? I would like to add an example of this method to the wiki. Thanks.

Martii commented 1 year ago

@reyaz006

Though it seems awkward to be required to move the whole code into a 3rd party function, but alright.

Just the already unfunctioned code is my educated guess for your script. The line numbers on the diff denote this hopefully but if not look at the surrounding statements. The trailing brace is just my editor adding a newline which is wise with .user.js engine managers. I probably should have turned that feature off when I created the post.

I did actually try it on TM/Chromium before your fork went away and it seemed happy from the console i.e. no exceptions thrown or log messages... but remember I don't know what your script actually does which is why I said you probably know it better than I.

sizzlemctwizzle commented 1 year ago

Updated the issue description and the wiki.

@Martii Nice catch on @grant in the wiki. I added those extra @grant lines to the wiki homepage as well. Without those in GM3 it would have dropped to localStorage.

Purfview commented 1 year ago

From wiki is not clear how this new behavior works. How to make sure that init() completes?

In one of my script there are 149 GM_config.get... It's not affected by "#92" issue.

In case someone needs quick fix for some old userscript, use this for // @require : https://cdn.jsdelivr.net/gh/sizzlemctwizzle/GM_config@43fd0fe4de1166f343883511e53546e87840aeaf/gm_config.js

Martii commented 1 year ago

@Purfview

It's not affected by "https://github.com/sizzlemctwizzle/GM_config/issues/92" issue.

Because the read and write functions are actually asynchronous (not the get function directly but related) timing is everything. If your init fails to finish before your first get you'll get a non-functioning script. My dev station is the fastest I could get from last year and I have some 4th generation systems for a slower test... so depending on your platform you could never see the issue, see the issue intermittently, or you could always see the issue.

This is the nature of asynchronous scripts and why get should ideally be called after init is confirmed to be finished.

@frank-trampe

I too would like to see your chaining reduced case test example in order to help you all better.

sizzlemctwizzle commented 1 year ago

@Purfview

How to make sure that init() completes?

From the wiki:

Now get needs to be moved into an event callback like init:

There is literally an event that fires when init() is completed. You just need to register a callback function. There are three examples of how to do this in the wiki.

It's not affected by "https://github.com/sizzlemctwizzle/GM_config/issues/92" issue.

Until this fix, GM_config had been using localStorage in Greasemonkey since version 4. Because of this fallback, it wouldn't seem like there was an impact.

frank-trampe commented 1 year ago

@sizzlemctwizzle, the only change necessary in my opinion is to make GM_config.init or some other arbitrary function return a promise that resolves when loading is complete. I actually thought that init was already doing that, but my code was instead randomly working for not immediately apparent reasons. For the moment, I have added a in the init configuration that resolves a previously declared promise.

sizzlemctwizzle commented 1 year ago

make 'GM_config.init` or some other arbitrary function return a promise that resolves when loading is complete

Or you could just create the promise yourself:

let onInit = gmc => new Promise(resolve => {
  let isInit = () => setTimeout(() =>
    gmc.isInit ? resolve() : isInit(), 0);
  isInit();
});

let gmc = new GM_config(
{
  'id': 'ThisConfig',
  'fields': {
    'val': {
      'label': 'Value',
      'type': 'text'
    }
  }
});

onInit(gmc).then(() => {
  // initialization complete
  // value is now available
  let val = gmc.get('val');
});

For the sake of consistency, I don't think GM_config should generate a promise for just one of its events, especially when the same thing can be accomplished by user code like the above example. We'll stick to the established event callback pattern.

Edit: Added a version of the above example that uses multiple thens to the wiki.

cyfung1031 commented 1 year ago

make 'GM_config.init` or some other arbitrary function return a promise that resolves when loading is complete

Or you could just create the promise yourself:

let onInit = gmc => new Promise(resolve => {
  let isInit = () => setTimeout(() =>
    gmc.isInit ? resolve() : isInit(), 0);
  isInit();
});

let gmc = new GM_config(
{
  'id': 'ThisConfig',
  'fields': {
    'val': {
      'label': 'Value',
      'type': 'text'
    }
  }
});

onInit(gmc).then(() => {
  // initialization complete
  // value is now available
  let val = gmc.get('val');
});

For the sake of consistency, I don't think GM_config should generate a promise for just one of its events, especially when the same thing can be accomplished by user code like the above example. We'll stick to the established event callback pattern.

Edit: Added a version of the above example that uses multiple thens to the wiki.

I just noticed this issue. I opened a separate issue to post my idea.

I think the solutions in the wiki page are not that straight forward. setInterval is not required. Bad coding to use setInterval as you already have init event. From the usage perspective, I agree that you should not change any existing method to break the code. Just add a new property and give a Promise object to let the external use await

then adding one line will solve the issue. await gmc.initialized;

see https://github.com/sizzlemctwizzle/GM_config/issues/126