erosman / support

Support Location for all my extensions
Mozilla Public License 2.0
170 stars 12 forks source link

[FireMonkey] GM_getValue/GM_setValue should be synchronous #98

Closed qsniyg closed 3 years ago

qsniyg commented 4 years ago

GM_getValue/GM_setValue are synchronous:

GM_setValue("variable", "test");
console.log(GM_getValue("variable"));
// logs 'test'

Looking at the source code (scriptAPI.js), it appears GM_setValue is just mapped directly to GM.setValue:

  script.defineGlobals({

    GM,
    GM_setValue:        GM.setValue,
    GM_getValue:        GM.getValue,
    GM_deleteValue:     GM.deleteValue,
    GM_listValues:      GM.listValues,
    GM_openInTab:       GM.openInTab,
    GM_setClipboard:    GM.setClipboard,
    GM_notification:    GM.notification,
    GM_xmlhttpRequest:  GM.xmlHttpRequest,
    GM_getResourceURL:  GM.getResourceUrl,
    GM_info:            GM.info,

    GM_fetch:           GM.fetch
  });

I'm not sure if GM_setValue needs to be changed, but GM_getValue returning a promise prevents some userscripts (including mine) from properly working. I'm guessing GM_listValues should also be synchronous.

Thank you for doing this by the way, it's great to have more options :)

erosman commented 4 years ago

The values are saved to extension storage and the storage operation, like most other operations, are asynchronous. There isn't any way to access storage synchronously.

It is the same case for GM4 where these operations are asynchronous. GM3 had synchronous operations but that was in legacy Firefox. Such operations are all asynchronous in FF57+.

Any script that is written for or need to run on Firefox 57+ needs to be updated for that.

GM.getValue

Returns

A Promise, rejected in case of error and otherwise resolved with:

qsniyg commented 4 years ago

@erosman Sorry, I meant that GM3's GM_getValue needs to return the value of the setting directly, instead of a promise returning the value (like GM4's GM.getValue does) :)

I believe the way Violentmonkey does it is that it caches the settings to the content script, allowing it to return the value directly without a promise, without needing to send a message to the background script.

If you don't plan on implementing this yet (considering that this might require restructuring the code), would it be possible to remove support for GM_getValue and GM_listValues in the meantime? Older scripts requiring it will fail regardless of whether or not it's implemented, as they don't expect a promise, and newer scripts that default to using GM_getValue over GM.getValue (but still support GM.getValue if GM_getValue is missing, this is the way mine works, as GM_getValue is faster in most userscript implementations) will be able to work properly.

erosman commented 4 years ago

I will look through the Violentmonkey but I doubt it.

The storage areas available to extensions are limited eg. local/sync/IndexedDB All of them are async in Firefox 57+

It does not mater if a script users GM.getValue or GM_getValue. As long as Firefox is 57+, the operation is async, One of the cornerstones of Quantum Firefox was to make all sync operations async so that they wont halt other operations while executing.

The only sync storage areas available as WebAPI (both page and extension) are localStorage & sessionStorage. They are not suited for this purpose as they would be accessible to other parties.

As content script is saved in storage then "caches the settings to the content script" is the storage unless it wants to keep them as temporary variables in RAM which is not feasible.

qsniyg commented 4 years ago

All of them are async in Firefox 57+

Yes, that's why Violentmonkey has to cache the userscript settings on the content script in order to allow GM_getValue to be synchronous (it returns the cached value, instead of querying browser.storage)

Here is the related code: https://github.com/violentmonkey/violentmonkey/blob/49dfdd02bbb4d27b0c55e30048d49babb702d0be/src/injected/web/index.js#L223

https://github.com/violentmonkey/violentmonkey/blob/49dfdd02bbb4d27b0c55e30048d49babb702d0be/src/injected/web/index.js#L369 This checks for store.values, which is populated at start, and when the values change (through a storage listener).

It does not mater if a script users GM.getValue or GM_getValue

Unfortunately it does, because the GM_getValue API is expected to return the value itself, while GM.getValue is expected to return a promise. This is why Greasemonkey had to create a new API instead of just updating the old one.

"caches the settings to the content script" is the storage unless it wants to keep them as temporary variables in RAM which is not feasible.

Sorry, I didn't explain it properly. The content script has a variable (store.values) where it caches all of the values when it's first loaded into a page, and updates them when the values are updated (through the webextension messaging system) :)

Thanks for looking into this!

erosman commented 4 years ago

I see ...

That is violentmonkey way of keeping all values in RAM. There are many problems with that. In computer crash, restart etc all those saved data are lost. Restoring session will result in old data coming up and not new.

Using sync/await is easy and it behaves like sync. It will also work with GM3 & GM4 and other managers. It will also work on Firefox 52.+

const value = await GM.getValue(key, default);
const value = await GM_getValue(key, default);
qsniyg commented 4 years ago

@erosman

That is violentmonkey way of keeping all values in RAM

No, it sets it to the storage immediately after: https://github.com/violentmonkey/violentmonkey/blob/49dfdd02bbb4d27b0c55e30048d49babb702d0be/src/injected/web/index.js#L252 (https://github.com/violentmonkey/violentmonkey/blob/49dfdd02bbb4d27b0c55e30048d49babb702d0be/src/injected/web/index.js#L381).

The RAM is just a cache mirror of the storage in order to allow the synchronous behavior to work. Whenever the value changes either way, it mirrors it back (if the storage is updated, it's mirrored to the cache, if the cache is updated, it's mirrored to the storage).

Using sync/await is easy and it behaves like sync.

Yes, it's a rather hacky workaround, but legacy scripts (that use GM_getValue) do not use await GM_getValue, meaning they will be incompatible with FireMonkey anyways.

It will also work on Firefox 52.+

That's the other issue, the userscript needs to work for older browsers without async support (and I am aware of a number of users that regularly use it with legacy browsers).


If you want, I can try to implement this in your addon and send you a patch. I understand that this might take a while to implement, due to the storage mirroring :)

erosman commented 4 years ago

I am able to write the code.. however... that misses the point completely.

Minimum Firefox version to use FireMonkey is 65.0 The API is not available before that. In fact the API became standard in FF 68.0

There is no point in trying to make FireMonkey compatible for something that was written for any Firefox older than 65.0 (especially something as old as before FF52)

qsniyg commented 4 years ago

There is no point in trying to make FireMonkey compatible for something that was written for any Firefox older than 65.0 (especially something as old as before FF52)

That's fine, but could you then remove support for GM_getValue? Because the implementation provided by FireMonkey is incorrect, meaning userscripts that try to support both will fail, as they expect GM_getValue to behave according to spec, but FireMonkey's behavior for the GM3 is completely different from that of other userscript managers.

Any legacy userscript using GM_getValue will not use await GM_getValue (even if they are written after async/await support, because GM_getValue is supposed to return the value, not a promise, according to the spec), so removing support for it will not further break any legacy userscript (in fact it might help users to understand why they don't work). For userscripts like mine, that support both APIs, it will allow them to work properly :)

Currently there's no way I can work around this issue on my end, as adding await will unfortunately break the userscript for people that use it with legacy browsers, there's currently no way for me to detect FireMonkey from my userscript in order to work around this issue (if GM_info is supported, I could work around it that way), and prioritizing GM.setValue over GM_setValue will cause performance issues for modern userscript handlers that implement GM_setValue according to spec (e.g. Violentmonkey/Tampermonkey).

To summarize:

One thing that might be good to note though, is that there are many userscripts that people still actively use (such as MPIV: https://greasyfork.org/en/scripts/404-mouseover-popup-image-viewer, probably one of the more popular userscripts), that require the legacy APIs to work according to spec in order to work properly (e.g. settings).

I hope this doesn't come across harshly, I appreciate your time looking into this :)

erosman commented 4 years ago

That's fine, but could you then remove support for GM_getValue? Because the implementation provided by FireMonkey is incorrect, meaning userscripts that try to support both will fail, as they expect GM_getValue to behave according to spec, but FireMonkey's behavior for the GM3 is completely different from that of other userscript managers.

FireMonkey is NOT a clone of other script managers. It has many features that other script mangers don't. Others also have features that are not supported in FireMonkey.

FireMonkey is a NEW script & CSS manger. All the necessary information is in the included help.

Adding a simple async/await will make GM_getValue synchronous, as explained in the Help.

It is easy enough to make scripts run on multiple script managers. I have 100s of script that can run on all script mangers. FireMonkey is for users of Firefox 65.+ which supports async/await. It has no relation to legacy browser users. It should not be expected to support scripts written for Firefox 30 for example.

There are many differences between current script managers, some of which are listed in the Help, e.g. the default run-at.

Detecting FireMonkey is also easy. AFAIK other script manger dont support GM_fetch so

const FM = typeof GM_fetch !== 'undefined'; or const FM = typeof GM_fetch === 'function';

You can also check this way, since FireMonkey is the only one supporting both at the same time: const FM = typeof GM.setValue !== 'undefined' && typeof GM_setValue !== 'undefined';

In your script, I noticed a check is performed for each GM function. Personally, what I have done is to create the GM. set of function if they dont exist and then map GM_ function to them, then uses async/await.

That way the script will run on any browser and any script manger that supports async/await which are listed at async function: Browser compatibility.

because GM_getValue is supposed to return the value, not a promise

FireMonkey GM_getValue also returns a value.

For old browser support, a try/catch (although not ideal) can be used to detect if async/await is supported by the browser.

qsniyg commented 4 years ago

FireMonkey is NOT a clone of other script managers. It has many features that other script mangers don't. Others also have features that are not supported in FireMonkey.

Fair enough, and that's great! Many other userscript handlers have been stagnating adding new features, which really is a shame, especially for a few features that would be very useful for many userscripts. I hope you can keep on adding new features as needed! :)

However, I would again personally argue that where they offer the same API, they should at least function somewhat similarly. FireMonkey's GM_getValue acts in a completely different way than other userscript handlers do, and as (from what I understand) it's meant to help legacy compatibility, I'm not sure how that's helpful.

I have 100s of script that can run on all script mangers.

Most that I have tried partially work under FireMonkey, but are unable to read their settings (due to this issue). For some, settings aren't too important, but for others, settings are rather crucial to the base functionality (including mine).

Detecting FireMonkey is also easy. AFAIK other script manger dont support GM_fetch so

Thank you!! I'll use typeof GM_fetch === 'function' && GM_info() === null for now then, and I'll try to monitor your addon in case this changes in the future.

That being said, if you do not plan on either removing the function, or fixing it to work according to the spec, would you at least consider at least partially implementing GM_info or having another concrete way to detect FireMonkey? This would really help me and other people trying to work around this issue, so that we don't have to rely on hacks that can break at any point in order to detect and work around FireMonkey's quirks.

Even just supporting scriptHandler would really help! That would then allow custom codepaths to be able to better detect FireMonkey's quirks, and possibly use FireMonkey-specific features as well, in a confident manner that doesn't rely on hacks :)

Personally, what I have done is to create the GM. set of function if they dont exist and then map GM_ function to them, then uses async/await.

As I wrote earlier, the reason I didn't do this is because of performance. The quicker the page can load, the more bandwidth is saved when redirecting images. Using GM.getValue by default is noticeably slower in other userscript managers.

FireMonkey GM_getValue also returns a value

Sorry, I misworded that. I meant that most other userscript handlers return the value of the setting directly, while FireMonkey maps the function to GM.getValue, which returns a promise that contains the value of the setting instead. I hope that makes more sense haha :)


On a sidenote, I'm really sorry if this has taken a more hostile tone, that was absolutely not my intention when opening this issue.

erosman commented 4 years ago

especially for a few features that would be very useful for many userscripts

Sure and I will add them if there is a popular demand and it is feasible. FireMonkey is still in its early days.

FireMonkey's GM_getValue acts in a completely different way than other userscript handlers do

true... but then how does GM_getValue act in GreaseMonkey4 for Firefox Quantum? The gm4-polyfill doesn't make it synchronous. Also ... Greasemonkey 4 For Script Authors

would you at least consider at least partially implementing GM_info or having another concrete way to detect FireMonkey?

TBH, I never found it useful personally HOWEVER, it is very easy to add the feature. Let me think about it and since someone else also ask for it, maybe I will add it.

qsniyg commented 4 years ago

The gm4-polyfill doesn't make it synchronous.

It works the other way around :) It creates a new function: GM.getValue from GM_getValue. In other words, it adds support for the GM4 API for userscript managers that only support the legacy API (it doesn't add support for the legacy API for userscript managers that only support the GM4 API).

maybe I will add it

Thank you for considering! This will help greatly to provide proper support for FireMonkey with the userscript :)

I don't care much about the reflection properties (finding the version/name of your own script), but knowing which userscript handler is running it (as well as the version, in case a quirk only appears in one version but not another) is super helpful for working around quirks like these to provide a good experience for as many users as possible :) So even if you just add support for those two variables (scriptHandler and version), it will be really useful!

erosman commented 4 years ago

OK.. I added GM.info/GM_info support but there is a bug in API handling 'run-at' (https://bugzilla.mozilla.org/show_bug.cgi?id=1583159)

I will update once something is sorted.

erosman commented 4 years ago

BTW.. in your code you first check (typeof GM_getValue !== "undefined") and then check (typeof GM !== "undefined" && GM.getValue)

If you change the order, then you wont need extra code to check GM.info for FireMonkey.

In fact, I would check it once and cache it at the start of script .e.g.

const GM4 = (typeof GM !== 'undefined' && GM.getValue); // promise based async API

// then

if (GM4) { .... }
else { .... }
qsniyg commented 4 years ago

@erosman Thank you, but as I wrote earlier, the reason I didn't do this is because of performance. The quicker the page can load, the more bandwidth is saved when redirecting images. Using GM.getValue by default is noticeably slower in other userscript managers (since asynchronous code is delayed for an undefined amount of time).

erosman commented 4 years ago

the reason I didn't do this is because of performance.

That should not be a factor here since except FireMonkey, others dont support BOTH so it is either this or that so there is no choice.

Also since the script is injected at @run-at document-start there is a LOT of time to get the values before content is loaded.

Anyway.. that was just a suggestion.

erosman commented 4 years ago

v1.16 uploaded with GM.info/GM_info support .. but read the Help

qsniyg commented 4 years ago

@erosman Thank you!

qsniyg commented 4 years ago

I'm not sure why this is marked as "done", the root issue (GM_getValue/GM_setValue working as GM.getValue/GM.setValue, which is incorrect) still remains...

erosman commented 4 years ago

I'm not sure why this is marked as "done"

You requested GM.info/GM_info support to be able to separate GM.getValue/GM_getValue and that was added. I assumed you can work with that.

Let's see if there is a popular demand for what you requested about GM_getValue (GM_setValue should not matter since the outcome is not required immediately).

erosman commented 4 years ago

synch GM_getValue

I have been thinking about it.

The way API works is that:

scriptAPI & user-script are injected automatically into the relevant page and have content scope.

In order for the user-script to have synchronous access to the stored data, the data must be available in content scope with means in scriptAPI or user-script itself. Any interaction with background script will be an async interaction anyway so caching it in the background script would mean async access.

That would mean injecting the actual storage value as a JS into the page e.g.: let cacheSstorage = {...}

I can write the code to make it possible but that would add some extra processing (read/write/update to cache as well as actual storage). There are GM_setValue/GM_getValue/GM_deleteValue/GM_listValues which used to by synch.

TBH, synch did not mean immediately even in GM3. The data still require read/write from/to file and that took time. The main difference was that the running code would not progress further until the process was completed.

Bearing in mind that GM_setValue belonged to pre Firefox 57 (2017-11-14) and Firefox userScript API was officially launched in Firefox 68, is it really worth adding all those extra processes!?? Currently, 99% FireMonkey users are using FF72+

There is also the fact that FireMonkey is not clone of other script managers and there are other differences as well which would mean not all user-scripts can run on FireMonkey without some adjustment.

qsniyg commented 4 years ago

It's completely fine if you don't implement it (and I completely understand your reasons), but then if so, please don't expose GM_getValue and GM_setValue, as any userscript that prefers the GM3 variation over the GM4 variation (for good reason) will be completely broken. If you don't expose those APIs, they will work if they have a GM4 fallback (as is the case for many modern userscripts).

As I wrote earlier, I personally believe there are two options here:

1: Implement GM_get/setValue to conform to spec (make them synchronous). GM3 scripts will work properly, regardless of whether they implement a GM4 fallback or not. 2: Remove GM_get/setValue. GM3 scripts that implement a GM4 fallback will be able to work.

The current solution (GM_get/setValue working in a completely different way than the spec) breaks both GM3 scripts that don't implement a GM4 fallback, and GM3 scripts that implement a GM4 fallback. In other words, there is absolutely no advantage to doing it this way.

which would mean not all user-scripts can run on FireMonkey without some adjustment

I understand that, but removing GM_get/setValue (if you don't want to implement it) will allow more userscripts to run out of the box. The current solution (having GM_get/setValue work contrary to spec) breaks every userscript that uses GM_get/setValue.

erosman commented 4 years ago

I am going to write the synch code and see how it works out ... let's see

erosman commented 4 years ago

So far, I have made ALL these synch (both GM. & GM_)

I thought it would be simpler to make both type the same. async/await will work nonetheless.

I have to test them out though.

erosman commented 4 years ago

I noticed a MAJOR issue with having synch storage. Data is no longer live.

If a script is running on multiple tabs (I have seen people with 1000+ opened tabs), the synch storage for each tab means it remains limited to the tab.

If storage is updated by one of the multiple opened tabs, the other tabs will not have access to the updated data until the tabs are refreshed.

Additionally, it would not be possible for the same script running on multiple opened tabs to share and exchange data.

!!!!????

qsniyg commented 4 years ago

@erosman The way VM does it is that whenever values are changed, it will update the values on all the tabs.

This would additionally make implementing GM_addValueChangeListener easier I believe.

erosman commented 4 years ago

The way VM does it is that whenever values are changed, it will update the values on all the tabs.

Where is it in their code?

qsniyg commented 4 years ago

https://github.com/violentmonkey/violentmonkey/blob/master/src/injected/web/gm-values.js

and

https://github.com/violentmonkey/violentmonkey/blob/master/src/injected/web/gm-api.js

erosman commented 4 years ago

From what it seems, each script would also require a listener to wait for the changes.

Bearing in mind that each tab can have many scripts running, which may include the multiple iframes, and then there might be many tabs open, updating the sync storage data:

It does not appear to be worth the extra resources and effect on browser performance it takes to make GM_getValue/GM_setValue synchronous for scripts written for GM3 (pre FF57 2017-11-14) and not yet updated.

qsniyg commented 4 years ago

That makes sense :)

Could you then remove GM_getValue/GM_listValues? As far as I know, there are zero GM3 scripts that use await GM_getValue, so although all userscripts that require those functions will be broken regardless of whether or not it's kept, removing GM_getValue etc. will allow scripts that use a GM4 fallback to work (which is actually a relatively significant amount of userscripts from what I've seen).

erosman commented 4 years ago

Personally speaking, I have 100s script myself and all run without error on FM/GM4/GM3/VM/TM.

All the scripts are written for GM4, and they are backward compatible (fallback to) GM3. (I dont use gm4-polyfill.)

Looking at Firefox user stats:

GreaseMonkey4       470,355 users
TamperMonkey        349,170 users
Violentmonkey        45,677 users
FireMonkey              419 users :p

It appears that majority are using GM4. It is not a problem to remove GM_getValue but then that doesn't solve other compatibility issues. As you are aware, there are differences between FM & other script managers.

Adding await does not break old scripts, and since the sync data is immediately available, it doesn't even delay the retrieval of data. (Note: async/await is FF52+)

Alternatively, changing the order of checking e.g. check for GM4 first is another option.

Let's also see how popular a demand there is for it.

qsniyg commented 4 years ago

Adding await does not break old scripts

It breaks compatibility with old browsers however, which is why I don't use it (and there are a number of users that still use very old browsers with my userscript). The only reason one would do this is to support FireMonkey, and for that, they would likely just use GM.getValue instead.

Modern scripts should be able to use both APIs, but many (not only mine, I have seen others) prefer the GM3 APIs due to the performance advantage.

erosman commented 4 years ago

v1.29 Added GM addValueChangeListener Added GM removeValueChangeListener

c01o commented 3 years ago

Hi,

I installed this add-on today, and immediately found some userscripts stopped working thanks to this incompatibility. I confirmed GM_getValue() still returns promise in FireMonkey 1.44, even though this issue has been closed with 'done' label. The label should be 'wontfix', shouldn't it?

Also, is there any workaround without manually adding await to every GM_getValue() call in the userscripts, which results in compatibility issue to the older browsers?

erosman commented 3 years ago

@c01o Changes to API process occurred in Firefox 57 (2017-11-14) and subsequently GM4 was released. Minimum Firefox version for FireMonkey is set as 68. Checking statists, there is a very low percentage of users who are using Firefox pre-57. There are many incompatibilities now with older scripts as well as new extensions and older browsers.

The implementation of synchronous GM_getValue() in some script-managers are work-around resembling synchronous and not a true synchronous process. There is no way to make GM_getValue() truly async in modern browsers.

Many userscript developers have not updated the script code structure for a long time.

Which script do you have problem with and what is the Firefox version that you are using?

c01o commented 3 years ago

@erosman Thank you for quick responce :) I know old userscripts can stop working for many reasons, and one of the problematic scripts is indeed very old one, but it seems that is not the root cause.

I'm sorry I'm hesitate to point the exact script having problem, but the script requires just 2 grants: GM_getValue and GM_setValue. Though no log output in both console and FireMonkey log screen, printf debug allow me to detect the problematic line, which calls GM_getValue. The script works perfectly on Firefox 82.0.1 + ViolentMonkey 2.12.7, but not for Firefox 82.0.1 + FireMonkey 1.44.

That script is on greasyfork.org, and not completely abandoned. Update comes annually or some. So I don't want to fork and fundamentally rewrite the script. Since the script is very old, the author won't willing to break its backward compatibility. Many userscripts can have the same kind of compatibility issues, so I need workaround on the engine side, rather than the script side. :)

Its very okay if you don't think the script compatibility is not so important, or even they are technical debt. But In that case, 'wontfix' label might be better for this issue.

erosman commented 3 years ago

@c01o The GM_getValue in other script managers works but it is not really sync. The data can not be read & updated asynchronously.

As you know yourself, it is easy to fix the issue by using async/await but script developers don't want to update the code because the script appears to work in VM/TM (but not in GM or FM).

Once a script that runs in multiple tabs tries to updated the stored value, then the failure of GM_getValue work-around will surface. The other script manager pass the storage to the script to be used by GM_getValue. There is not a problem there. The problem is that those values can not be updated asynchronously, so if 2 tab with the same script try to get & set value, the result will become unpredictable.

That is the reason any async GM_getValue implementation is only a pseudo-measure.

If there weren't any other option, then a pseudo-measure could be considered, but since there is a simple async/await, why not?

qsniyg commented 3 years ago

The GM_getValue in other script managers works but it is not really async. The data can not be read & updated asynchronously.

Why does it need to be an async function, when it can be stored in a map that is updated when the value changes (through a listener)? This is how VM and TM do it, as well as my extension. This not only works without issue, it allows it to actually be much faster than the async function, as it's just an object lookup, without any kind of IPC required (while still remaining correct for multiple tabs modifying the same values, because of the value change listeners).

so if 2 tab with the same script try to get & set value, the result will become unpredictable

It is entirely predictable if you use the listener technique I mentioned earlier. Unless they get/set it at exactly the same time (in which case, both this and the async GM.getValue will be unpredictable).

I've done very extensive testing with my script under both VM and my addon (which overall uses the same technique, though in a slightly different way). There haven't been any issues regarding multiple tabs running it, while modifying the same values, which makes sense (algorithmically speaking, the only way an issue could happen is if there was a bug on the browser's side).

but since there is a simple async/await, why not?

This breaks compatibility with older browsers. One of the main advantages of userscripts is that they can be run on any browser, including ancient or mobile ones (which, according to a number of users of my script, is surprisingly common). As I mentioned earlier, the only reason a developer would ever use await GM_getValue is to add compatibility with FM, which breaks compatibility with older browsers.

erosman commented 3 years ago

Why does it need to be an async function,

Sorry, typo.. I meant sync .. corrected

updated when the value changes (through a listener)

Listener is not sync and it is async.

It is entirely predictable if you use the listener technique I mentioned earlier. Unless they get/set it at exactly the same time

That is what I meant, tabs trying to set data at the same time.

(in which case, both this and the async GM.getValue will be unpredictable).

Not the case, since the async operation is queued in sequence from the background process.

This breaks compatibility with older browsers

async/await has been available since Chrome 55. Edge 15, Firefox 52. How old a browser is considered for backward compatibility?

What about GM4 users? The sync GM_getValue doesn't work in GM4 either.

There are a number of other differences between the current available script managers (FM|GM|VM|TM) and each have their own pros & cons. Users choose the one most suited to their requirements.

True synchronous getValue/setValue is not possible, otherwise I would have been happy to implement it.

qsniyg commented 3 years ago

That is what I meant, tabs trying to set data at the same time.

I understand, and you're right, async would be the only proper fix for that issue.

In that case, I would recommend removing GM_getValue, see reasoning below:

What about GM4 users? The sync GM_getValue doesn't work in GM4 either.

Which is fine, because userscripts that support both GM.getValue and GM_getValue but prefer GM_getValue (like mine, as well as many others) will therefore work fine under GM4, because GM_getValue will not be called.

There are a number of other differences between the current available script managers (FM|GM|VM|TM) and each have their own pros & cons. Users choose the one most suited to their requirements.

That's fine, however, as I've mentioned before, FM's GM_getValue implementation is like a browser implementing confirm with a promise (e.g. confirm('...').then(function() { })). This does not improve compatibility, and breaks any website that uses the function (and in this case, there's a properly implemented backup function, GM.getValue, which functions correctly, but scripts that prefer GM_getValue over GM.getValue therefore won't work). await GM_getValue will never be called by a userscript, unless they're explicitly trying to support FM (and don't care about breaking compatibility for older browsers), in which case, they might as well use GM.getValue instead (like mine does), which not only works according to spec under FM, but also doesn't necessarily break compatibility with older browsers either.


To be clear: I'm talking about this for other userscripts. For my userscript, I don't personally particularly care about whether GM_getValue is implemented properly or not, I've added a rather significant amount of code for detecting FM and working around its quirks, I just treat it as if it were a broken website/addon interfering with the page. (By the way, thank you again for implementing GM_info, it would have been much harder to do this if it hadn't been implemented :) )

erosman commented 3 years ago

I am going to see if it is possible to implement a quasi-synchronous GM_getValue :thinking:

erosman commented 3 years ago

It is done and I am testing... for v1.45

const value = GM_getValue(key, default);          // sync
const value = await GM.getValue(key, default);    // async Promise

const keys = GM_listValues();                     // sync
const keys = await GM.listValues();               // async Promise
erosman commented 3 years ago

v1.45 uploaded

erosman commented 3 years ago

Actually, the sync GM_getValues is broken. I have to revert back to the old set-up for now. :( ref #255

erosman commented 3 years ago

I have not found a method to pass the script storage to the script before it is inserted in order to make it available synchronously to the script. Script storage data is in extension storage and getting it is asynchronous.

My previous attempt (v1.45-1.46) failed and caused problems.

If anyone has a suggestion, I can work on it.

qsniyg commented 3 years ago

Sorry if I'm misunderstanding the issue, but how about having a sort of _internal_values global set in [userscript].defineGlobals, which contains the initial storage, which GM_getValue would then initially read from? I'm not quite sure how you implemented GM_getValue (I can't seem to access 1.45/1.46 from AMO to check, I'm guessing you disabled those versions?), so maybe this wouldn't work with your implementation.

Edit: Ah, if you're instead referring to the defineGlobals needing to be called without async, what about caching the storage to your extension background script whenever they're set? That way it can be accessed synchronously.

erosman commented 3 years ago

I can't seem to access 1.45/1.46 from AMO to check, I'm guessing you disabled those versions?),

Yes.. GM getValue/setValue were broken in those so I disabled them.

Sorry if I'm misunderstanding the issue, but how about having a sort of _internal_values global set in [userscript].defineGlobals, which contains the initial storage, which GM_getValue would then initially read from?

In FireMonkey using the dedicated API, those variables are set at script register time. Typically, that is when browser is started and/or when script is enabled. After registering a script, the extension doesn't do anything else with the script and all is handled by Firefox. That is how the dedicated userScript API works and since the process is handled by Firefox (not manually by the extension), it is more efficient.

Therefore, while it is possible to add script storage data to the script, the data would be old (e.g. at the time of browser start) and can not be updated.

Other user script mangers manually inject script into the tab. That would mean:

Besides the overheads of doing above, it also takes time to process. For example, the @run-at document-start in the dedicated Firefox API is the true document-start.

After injecting a script and its storage data, then any further update to its storage data will still be asynchronous since there is no way to pass the new data to the script synchronously.

In the failed attempted (v1.45-1.46) I tried to inject the script storage into the script as script is injected. However that failed since the process was still async and the script that sets or gets values immediately (as opposing to later after a timer or after an event listener) would run before the data is received.

One script manager also uses localStorage which is synchronous, as an option. The problems with localStorage are:

Ah, if you're instead referring to the defineGlobals needing to be called without async, what about caching the storage to your extension background script whenever they're set? That way it can be accessed synchronously.

Background script in my extensions always cache the storage data for sync & fast retrieval anyway. The problem is passing it the script. As mentioned above, it is only possible at register time. Any attempt to pass it to the scripts later would require an async process.

qsniyg commented 3 years ago

Got it, sorry I'm not familiar with the userscript API, I wasn't aware that was the case.

Perhaps then a ticket could be opened? E.g. adding an updateGlobals function could perhaps work (thought it might lead to some performance issues)

erosman commented 3 years ago

Perhaps then a ticket could be opened? E.g. adding an updateGlobals function could perhaps work (thought it might lead to some performance issues)

I have already discussed it with Mozilla engineers multiple times. There is no interest to create any sync operation as it will affect the browser performance (sync means browser has to halt other operations while async means it is queued for the next available cycle).

In fact, user-script developers are sticking to the old sync code. VM|TM (but not GM) have kept the compatibility at a cost of performance to keep the user-script developers happy.

The delay is there as far as running the script is concerned.

The quasi-sync get the storage data (delay here) and make it available to the script but after that the script can get the data without delay. The manual insertion of the user-script creates additional delay on top of that.

The async method runs the script without delay but there is delay when getting storage data.

Both have a delay. In fact running the script without delay works better for most script, especially those that do not use script storage immediately on loading.

Furthermore, the delays is often 2 milliseconds. so await GM.getValue(key, default) may take few milliseconds to resolve which is insignificant when tabs often takes 1000s of milliseconds to load.

Comparison

ℹ️ In brief ...

chocolateboy commented 3 years ago

TM supports GM_ but doesn't appear to support GM.

I can't see it mentioned in the documentation, but it does support both.