Okay, thanks. The fix is in 1.2.3
1 Like
@rfindley
Can I request a feature related to on_pageload?
I would like for scripts to have a simple way to handle the user exiting the desired page.
In particular, this is useful for any listeners added or modifications made to window or document.body. Think of it like a finally block, where script cleanup can occur.
You may want to read my recent post(s) in the Jitai thread if you want a better idea of where I’m coming from.
Theoretically, this could be quite elegant, such that the firing of the on_pageload callback acts as an on switch for this second callback, which is then fired once and switched back off when the page navigates somewhere that fails the regex for the earlier callback, and/or perhaps it just checks if the previous URL matched the regex (and the upcoming URL does not). Though a more basic example could just have it act as a negative version of on_pageload’s regex test. The latter would result in more calls, which could technically have performance side effects and is a bit undesirable from a conceptual standpoint, but it also is probably much easier to create, and something is better than nothing, so that’s why I am proposing both.
I had put a lot of thought into that while making the on_pageload, and ultimately decided to wait until someone requested it. That way, I could get a fix out the door faster, and I wouldn’t spend time on something until I knew people actually wanted it.
Now that it has been requested, I will revisit what I was thinking. I currently have some optimizations that aren’t compatible with doing ‘exit’ calls, but it’s not a problem to change it.
1 Like
New version of Open Framework (v1.2.4):
- Added try-catch blocks around pageload callbacks to stop errors in one script from affecting other scripts.
- Added support for
unload_handlerparameter towkof.on_pageload:
Example1:
wkof.on_pageload(
[ // The dashboard has two possible URLs
'/',
'/dashboard'
],
load_ultimate_timeline,
unload_ultimate_timeline //(optional)
);
Suppose you start on /dashboard. wkof will call:
load_ultimate_timeline("/dashboard", 1);
// The "1" means that the second URL pattern was matched.
Then if you click on the WaniKani logo in the upper left, which navigates to “/”, wkof will call:
load_ultimate_timeline("/", 0);
// Again, the "0" means that the first URL pattern was matched.
If you then click on the Reviews button, wkof will call:
unload_ultimate_timeline("/", 0);
2 Likes
The on_pageload function broke for me. It looks like there is a typo on line 366 wich should be regex = regex_entry.regex; I think, instead of regex = cache_entry.regex;
Fixed! Thanks for the heads-up. Looks like I had a hole in my test cases.
1 Like
@rfindley, I made some suggestions in the Double Check thread, but I saw some additional areas for improvement, so I went ahead and worked those out.
Here’s a git patch of the suggested changes (compared to the current greasyfork version)
diff --git a/Core.js b/Core.js
index 0f3ed75..97b945f 100644
--- a/Core.js
+++ b/Core.js
@@ -81,7 +81,7 @@
function split_list(str) {return str.replace(/、/g,',').replace(/[\s ]+/g,' ').trim().replace(/ *, */g, ',').split(',').filter(function(name) {return (name.length > 0);});}
function promise(){let a,b,c=new Promise(function(d,e){a=d;b=e;});c.resolve=a;c.reject=b;return c;}
- function is_turbo_page() {return (document.querySelector('script[type="importmap"]')?.innerHTML.match('@hotwired/turbo') != null);}
+ function safe_promise(func, ...argArray) {return typeof func === 'function' ? new Promise((y,n) => {try {y(func(...argArray));} catch (e) {console.error(e); n(e);}}) : Promise.reject(Error('First argument must be a function'));}
//########################################################################
@@ -344,6 +344,7 @@
let pgld_req = {url_patterns, load_handler, unload_handler, pattern_idx:-1, next_pattern_idx:-1};
pgld_req_store.push(pgld_req);
+ let load_promises = [];
url_patterns.forEach((pattern, pattern_idx) => {
let regex;
if (typeof pattern === 'string') {
@@ -351,7 +352,7 @@
} else if (pattern instanceof RegExp) {
regex = pattern;
} else {
- pgld_req.regexes.push(null);
+ // pgld_req.regexes.push(null); // not sure what was the plan here, since `regexes` property is undefined
return;
}
@@ -369,11 +370,10 @@
if (pgld_req.pattern_idx !== -1) return;
if (regex.test(current_page)) {
pgld_req.pattern_idx = pattern_idx;
- try {
- load_handler(current_page, pattern_idx);
- } catch(e) {}
+ load_promises.push(safe_promise(load_handler, current_page, pattern_idx));
}
});
+ return Promise.allSettled(load_promises)
}
//------------------------------
@@ -381,6 +381,7 @@
//------------------------------
let turbo_load_fired = false;
function handle_pageload(event) {
+ pageload_timeout = null;
let last_page = current_page;
if (event) {
turbo_load_fired = true;
@@ -400,30 +401,32 @@
});
});
+ let promises = [];
// Call all 'unload' handlers.
pgld_req_store.forEach(pgld_req => {
// If page was active, but not anymore, call the unload handler
- if ((pgld_req.pattern_idx !== -1) && (pgld_req.next_pattern_idx === -1)) {
- try {
- pgld_req.unload_handler(last_page, pgld_req.pattern_idx);
- } catch(e) {}
+ if (pgld_req.unload_handler != null && (pgld_req.pattern_idx !== -1) && (pgld_req.next_pattern_idx === -1)) {
+ promises.push(safe_promise(pgld_req.unload_handler, current_page, pgld_req.pattern_idx));
}
pgld_req.pattern_idx = pgld_req.next_pattern_idx;
pgld_req.next_pattern_idx = -1;
});
-
- // Call all 'load' handlers.
- pgld_req_store.forEach(pgld_req => {
- if (pgld_req.pattern_idx !== -1) {
- try {
- pgld_req.load_handler(current_page, pgld_req.pattern_idx);
- } catch(e) {}
- }
+ return Promise.allSettled(promises).then(() => {
+ promises.length = 0;
+ // Call all 'load' handlers.
+ pgld_req_store.forEach(pgld_req => {
+ if (pgld_req.load_handler != null && pgld_req.pattern_idx !== -1) {
+ promises.push(safe_promise(pgld_req.load_handler, current_page, pgld_req.pattern_idx));
+ }
+ });
+ return Promise.allSettled(promises);
});
}
+ let pageload_timeout = null;
function delayed_pageload(event) {
- setTimeout(handle_pageload.bind(null, event), 10);
+ if (pageload_timeout) {if (!event) return; clearTimeout(pageload_timeout);}
+ pageload_timeout = setTimeout(handle_pageload.bind(null, event), 10);
}
//########################################################################
Edit: Changed some semantics to mimic the current behavior—guaranteeing that all of the unload handlers are run to completion before any of the load handlers are started.
Edit2: I had forgotten to edit in a change I had made since originally posting, which avoids attempting to add a new promise to be called when the function was not set.
Also…it seems there was an error with encoding when I copied the diff over directly. I fixed the first line now, but I’m gonna see if there were any other spots that got mangled. and it seems like that was the only problem line.
Thanks! I’ll try to look it over as soon as I can. I’m traveling this week, though, so it will probably be next week at the earliest. Feel free to ping me as a reminder. I haven’t been on the WK forums much lately.
Heya @rfindley. I made a couple small edits to the above proposed changes and made a note about them.
Anyway, I’d also like to mention an issue regarding the Menu module.
The fastest way would probably be to just show you.
As you can see, if one installs over a certain number of scripts, it will expand past—and more importantly, beneath—the quiz buttons.
From what I could tell, the .additional-content class has z-index: 9; set, so by changing the .character-header {z-index:1;} defined in scripts_submenu style to 10, it appears this can be easily rectified (also, due to stacking rules that I just now started learning about, this seems like one of the only ways to fix it, though I may be completely wrong about that).
I’m a bit hesitant about suggesting a magic number though, since that could easily change. Ideally, it’d be something like…
(document.getElementById('additional-content')?.computedStyleMap().get('z-index').value || 9) + 1
1 Like
As always, thanks for your investigation and solutions!
Without having set up a reminder for myself, obviously I had forgotten to follow up on your prior post after returning from travel. This time, I’m leaving Discourse’s email notification marked as ‘unread’ in my inbox’s WaniKani folder so I will remember to work on it. 
2 Likes
I’m hesitant to have the framework wait on all “unload” handlers because that allows a broken script’s unloader to inadvertently prevent the “load” handlers from being called, which would break all other scripts.
I’m guessing you had a case where your unloader was async? I’m wondering if there is another solution we could consider.
To be clear, that particular comment was in reference to the edit that I made in order to make the behavior work the same as the way you had/have it.
tl;dr My assumption was that the order of “run all unload handlers → run all load handlers” was important and the edit fixed an oversight in regard to that on my initial post.
To explain, let me back up a bit.
This is the current (and previous) way it’s being called.
// Call all 'unload' handlers.
pgld_req_store.forEach(pgld_req => {
// If page was active, but not anymore, call the unload handler
if ((pgld_req.pattern_idx !== -1) && (pgld_req.next_pattern_idx === -1)) {
try {
pgld_req.unload_handler(last_page, pgld_req.pattern_idx);
} catch(e) {}
}
pgld_req.pattern_idx = pgld_req.next_pattern_idx;
pgld_req.next_pattern_idx = -1;
});
// Call all 'load' handlers.
pgld_req_store.forEach(pgld_req => {
if (pgld_req.pattern_idx !== -1) {
try {
pgld_req.load_handler(current_page, pgld_req.pattern_idx);
} catch(e) {}
}
});
This is synchronous. It calls all the unload handlers and then all of the load handlers. And furthermore, both of these are done inside of a loop, so each script is required to wait until the previous script finishes work before it can begin, in a first-in-first-out manner, all on the stack.
My idea for optimization was to simply generate a list of function calls and then wait for all of them to complete, essentially, ignoring their ordering for the purpose of efficiency. However, after my initial post, I remembered that the action of creating a promise itself ends up starting the function call (there’s a workaround for that, but it seemed a bit excessive here), which meant that these lines
return Promise.allSettled(unload_promises)
.then(() => Promise.allSettled(load_promises));
were not guaranteed to mean that it would happen in that order. In fact, the only situation in which that would happen is if all of the unload handlers were done by the time they were finished being assembled.
So, I changed it so that the creation of the load handlers array happens after the unload handlers have settled. That may have been unnecessary, but I didn’t want to assume that it would be safe for all handlers (both unload and load) to run in any order.
If that assumption doesn’t hold, however, then it makes it much simpler, really, since all of the handlers could be added in a single loop and awaited together that way.
And I’m pretty sure it would also deal with the issue you brought up:
(Though, correct me if I’m wrong, but isn’t what you mentioned the way it currently works as well?)
Which could look something like this:
let promises = [];
pgld_req_store.forEach(pgld_req => {
// If page was active, but not anymore, call the unload handler
if (pgld_req.unload_handler != null && (pgld_req.pattern_idx !== -1) && (pgld_req.next_pattern_idx === -1)) {
promises.push(safe_promise(pgld_req.unload_handler, current_page, pgld_req.pattern_idx));
}
pgld_req.pattern_idx = pgld_req.next_pattern_idx;
pgld_req.next_pattern_idx = -1;
// Get all 'load' handlers.
if (pgld_req.load_handler != null && pgld_req.pattern_idx !== -1) {
promises.push(safe_promise(pgld_req.load_handler, current_page, pgld_req.pattern_idx));
}
});
return Promise.allSettled(promises)
But alas, whether that’s what you want is for you to decide.
The above has all been about enabling asynchronous behavior, I guess.
There’s another small change involved in my post, however, that I never really pointed out. It’s packed away in my safe_promise() function. So, the current implementation consumes and silences any and all errors from the function calls. A little console.error(e) in the catch statement would be nice to have for debugging other scripts.
You could also make the resulting stack trace one line shorter by swapping the forEach loops for for...of loops (which takes only a few seconds to convert[1]). 
for (const pgld_req of pgld_req_store) { ... }↩︎
The last version of the open framework has reintroduced the problem that you need a refresh before any scripts will work. Before this version this happened very rarely, but now it happens nearly every time for me. Is anyone else seeing this, or are some of my scripts causing a weird race condition?
This happens when opening the reviews page from the dashboard, which will then not load doublecheck, or any other script relying on the on_pageload function. I have version 1.2.9 installed.
1 Like
I have done some debugging on my end to make sure I was not imagining things, and I found the problem. in the if 'doc ready' part of the delayed_pageload function, the effects of a turbo:load event are simulated if the document is ready before a turbo:load event is observed. Then a variable is set to skip the next turbo event to make sure nothing is loaded twice. This however makes the implicit assumption that another turbo:load event will eventually come before a new page is loaded, and this assumption is false on my machine. It regularly happens that the dashboard is ready before a turbo:load event is generated, but then when I move to the reviews page the load for that page is skipped instead.
I think this could be solved by keeping track for which page you want to skip the next load, instead of skipping all of them.
@gijsc1, thanks for the investigation! I think I may have this fixed now, implemented the same way you suggested. I was unable to test it directly since I can’t recreate the race condition, but I did run some simulated conditions, so hopefully it works. If not, let me know.
Thanks again!
1 Like
In the settings dialog I need a callback that is called when the X icon on the top right of the dialog is clicked. What should i use? The closest I can find is on_close callback but this is also called when to dialog is closed on clicking the save or cancel button. This won’t do.
There are four events:
pre_openon_saveon_cancelon_close
The order of events is:
pre_open(always called upon open)on_saveoron_cancel(if one of those buttons is clicked, but not if ‘x’ is clicked)on_close(always called upon close)
I would suggest initializing a flag in the pre_open event, then setting the flag if either on_save or on_cancel is called. Then, when on_close is called, check if the flag is set. If it’s not set, the user clicked ‘x’.
1 Like
Thanks. I will try this.
It works like a charm. Thanks again.
1 Like
@rfindley I need help with the ApiV2.get_endpoint() caching. The cache is stored in two places: 1) in the indexdb database and 2) in memory in the ep_cache object. The cache is valid for 60 seconds by default.
I need to ignore the cache when retrieving data through the ItemData.get_items() function. How can I do this? file_cache.delete() doesn’t work because the ApiV2.get_endpoint function retrieves the data from the in memory cache whis is unaffected by file_cache.delete() .The option parameter force_update doesn’t work because I call ItemData.get_items() and it doesn’t pass options to the ApiV2.get_endpoint() function. What should I do?
The need to perform a forced update occurs when the user does a short review session (less than a handful of items) that takes less than 60 seconds to complete. The sequence of event is:
- The user refreshes the browser to get the items in the review button. The initializes the cache and starts the 60 second timer.
- The user does the reviews and completes before the expiration of the 60 second timer.
- Wanikani quits the review module and return to the dashboard. This cause a Turbo page load event.
- In reponse to the Turbo event scripts load the data using
ItemData.get_items()to get the data with the modifications done by the review. They don’t want the cache data but get it anyway because the timer is not expired.
I think letting scripts to manipulate the cache in ItemData.get_items() is a bad idea because lots of script wants to get the same items data all at once. It they all invalidate the cache there is no point in caching. In an ideal world the framework would detect the page load event and clear the cache before notifying the scripts the page load occurred.