What a pleasant surprise!
As always, thanks for your continued support.
I took a quick peek at the new script and figured I’d point out what’s probably just a small oversight.
function on_pageload(url_patterns, handler) {
let call_handler = false;
if (!Array.isArray(url_patterns)) url_patterns = [url_patterns];
url_patterns.forEach((pattern, pattern_idx) => {
let regex, regex_string;
if (typeof pattern === 'string') {
regex = new RegExp('^'+pattern.replace(/[.+?^${}()|[\]\\]/g,'\\$&').replaceAll('*','.*'));
} else if (pattern instanceof RegExp) {
regex = pattern;
} else return;
regex_string = regex.toString();
let pgld = pageload_regex[regex_string];
if (!pgld) {
pageload_regex[regex_string] = pgld = {regex: regex, handlers: []};
}
pgld.handlers.push({fn:handler, idx:pattern_idx});
if (regex.test(last_page)) handler(last_page, idx);
});
}
The last idx will be undefined here. Perhaps you meant to use pattern_idx?
Also, the call_handler at the start is unused. Not sure if you simply forgot to remove it after a refactor or something.
Here’s another thing for consideration. I don’t know the exact performance difference between creating a URL object vs creating a RegExp object, so if the URL creation is worse then ignore the following. This line in handle_pageload(event)
Thanks for the code review!! My window of free time ran out last night, so unfortunately the quality checks were minimal . Tested, but not cleaned up. But the WK community is full of fantastic people such as yourself, so a big thanks to everyone who is patient, kind, and helpful!
Especially, thanks for the URL class tip. I learned Javascript before most of the APIs existed, so much of my code is admittedly “old-skool”, and I have been slow to catch up on JS’s modernization since I am mostly a C/C++ developer (and just hardware design, lately). But Someday™ I hope to have a web-focused project again where I can bring my JS coding up to date. It will probably end up being a home project, though. Like a home-automation coffee table command center .
Anyway, I’ll wait a few days for any bugs to be revealed, then I’ll (try to remember to) push an update with your corrections/suggestions above. Thanks again!
Heyo, seems like the recent update kind of broke the script, at least on my side:
Uncaught (in promise) ReferenceError: Unhandled Promise Rejection: handle_page_events is not defined
at Wanikani Open Framework.user.js:629:34
I think the problem lies here:
function startup() {
global.wkof = published_interface;
// Handle page-loading/unloading events.
if (is_turbo_page()) {
try {
document.documentElement.addEventListener('turbo:load', handle_pageload);
} catch(e){}
} else {
ready('document').then((e) => handle_page_events());
}
// Mark document state as 'ready'.
if (document.readyState === 'complete') {
doc_ready();
} else {
window.addEventListener("load", doc_ready, false); // Notify listeners that we are ready.
}
// Open cache, so wkof.file_cache.dir is available to console immediately.
file_cache_open();
wkof.set_state('wkof.wkof', 'ready');
}
as the method still tries to call handle_page_events(). Changing it to the renamed method handle_pageload() seems to have fixed it (at least on my end).
As I haven’t read the code I, I hope I’m not stepping on anyone’s toes with this, just trying to give a heads-up!
That’s a good catch, though I’m also rather curious how you even encountered the error.
After thinking about it, do you have Tampermonkey’s inject mode set to instant? I was able to reproduce the issue that way for myself. Kinda throws a wrench in the mix, but I think this should definitely be addressed.
@rfindley Their issue is quite problematic, because the “turbo:load” listener will never be added since the script is injected before the page itself has imported Turbo. You may have a more elegant solution for this, but my first thought would be to add the event listener regardless of whether Turbo currently exists.
I actually have it set to ‘auto’ for whatever reason, despite that not being the default. Can’t say I remember why. (I’m using Chrome, just for clarity)
Edit: I just realized you meant Tampermonkey, which I’m not using. I’m on Violentmonkey instead… Whoops
No worries, I kinda gathered that from the screenshot. In fact, I shouldn’t have hastily assumed you were using Tampermonkey. Regardless, it’s useful feedback.
Indeed, good catch. I’ll push the update (and @Inserio’s) shortly.
Hmm… I don’t know anything about “Instant” Inject Mode. Can you point me to its documentation? I can’t find any documentation, presumably since it is still experimental. (Or just because all search engines have turned to crap)
Haha I don’t know really anything about its inner workings either besides what I have assumed from the name. I only thought about it because I’ve fiddled with it when testing out various scenarios in the past and encountered different behavior. Though there’s some info at Stack Overflow.
In any case, my statement in which I tagged you was in reference to the fact that they were hitting the else branch, which only occurs when the script thinks that Turbo doesn’t exist. How they got there was less of a concern, all things considered.
I’ve posted a new update for the cleanup and bugs noted this morning.
@LordGrox, what page were you on when that occurred?
@Inserio, What is supposed to happen is: wkof looks for an import-map that makes reference to hotwire. Since the import-map is directly in the top-level html (rather than loaded dynamically), it should always detect Turbo correctly. And even though wkof runs before Turbo starts, it shouldn’t matter because it’s just adding a listener for any future events named “turbo:load”. But maybe there’s something I haven’t thought of…
That’s just the thing. When I put a debugger breakpoint on the line where it checks for is_turbo_page() while using Inject Mode: Instant, I was able to consistently (at least, currently, on my laptop) reproduce it returning false, because the entire document is virtually empty and no scripts have even begun to be imported.
I’m not sure if this is reliable enough as a way to determine if the state matches the situation I’m describing, but I can confirm that document.readyState is loading.
Anyway, here’s one possible solution that uses a technique I was relying on with the Turbo Events library.
Yeah, I forget that nearly every time. I wish Javascript could (easily) read the @version tag in the header!
Yeah, that makes sense if my assumption of what “Instant” does is correct. But that’s also sort of overriding the @run-at, so it makes sense that it would break. I’m wondering why Inject Mode is even a global setting rather than just being a new @run-at option.
Anyway, I’ll experiment with the snippet you posted and see if I can spot any gotchas.
While you’re at it, see if you also get weird behavior of the settings menu in reviews when doing the following (I think it’s unrelated to the current particulars under discussion).
Dashboard → start Reviews → Home nav button or Browser back → start Reviews (again).
For me, I get something like this.
If you can’t reproduce it, I’ll check back later and see if there’s anything I can do to assist.
I encountered that during initial development and thought I had it working before the initial release. Basically, it needs different CSS for the Lessons/Reviews/Extra Study pages versus the rest of the site. It’s supposed to erase the CSS and repopulate when you navigate, but it’s apparently not bullet-proof yet.
@LordGrox, what page were you on when that occurred?
is_turbo_page() returning false happens on both the dashboard as well as the review page as far as I can tell.
Just did some fiddling to see if its any of my local browser addons but didn’t see any difference. I turned all of my addons off for testing, and what I found was that it returns false in about 50% of page refresh’s, in the other 50% it did not.
I tried messing with the injection mode settings as well but spotted no difference…
I’ve seen that intermittently a couple of times too, just started in the last week or so (but before the open framework update) (and I don’t have double check or any other scripts on the reviews)
Always install “turbo:load” listener. When doc is ready, check for window.Turbo. If not present, manually call handle_pageload().
The Menu module previously had separate CSS for reviews/lessons/extra_study -vs- everywhere else. Now, I’ve combined the CSS, and I inject it only once at the initial pageload, and the CSS now uses a class selector to differentiate styles for different areas of the site.
I tried using the on_pageload functionality and it genereally works, but I noticed that when refreshing the page sometimes it doesn’t work. Since a lot of scripts currently still rely on manually refreshing to work, this is a problem.
After adding some debug statements it looks like sometimes when refreshing, the installation of all the listeners happens, but they are not fired. I think this might be because of a race condition where sometimes the listeners are added after the turbo:load event has already fired. I fixed it for my script with the following stopgap solution, but I am hoping that there is a better way to solve this within wkof.
//Hopefully temporary code to deal with registering after the load event has already fired.
setTimeout(()=>{
if(!not_loaded && window.location.pathname == target_path)
{
load_script();
}
}, 500)
I updated Open Framework sometime late last night / early morning to attempt to fix this issue. Do you know if you had the lastest version when it was misbehaving?
. Tested, but not cleaned up. But the WK community is full of fantastic people such as yourself, so a big thanks to everyone who is patient, kind, and helpful!
.
