Convert plugin loading to async/await forward JS #321

nrn · 2024-07-11T17:28:49Z

The main reason for this conversion is to contain errors on a per plugin basis so one broken/missing plugin doesn't break the rest of the page.

dobbs

Thanks for working on this, Nick. Thanks especially for separating the conversion to javascript from the conversion to async...await.

I've made several comments to the second of those commits.

dobbs · 2024-07-11T23:33:48Z

lib/plugin.js

-    if (itemElems.length === 0) { return promise; }
-    const itemElem = itemElems.shift();
+
+  for (const itemElem of $items.toArray()) {


noticing out loud that I like the change from a recursive call to emitNextItem() to a for...of loop.

dobbs · 2024-07-11T23:41:22Z

lib/plugin.js

        $item.off();
-        return plugin.emit($item.empty(), item, () => resolve());
-      }));
+        await plugin.emit($item.empty(), item)


This looks like it lost a call to resolve(). Was it only being used to help with the ordering in the previous code? Put another way, does the await at the front of this line replace it?

Yeah, the await is a pretty direct replacement for the callback w/ resolve, the main difference is an error thrown in plugin.emit would now surface here and get caught. I left the callbacks on all of these functions available but optional, since they are exported, but with promises flowing through all of them now we only have to do the new Promise(function (resolve... thing at the edges where this file interacts with external callback functions.

dobbs · 2024-07-11T23:44:02Z

lib/plugin.js

+      } catch (e) {
+        console.error(e)
+      }


Am I reading the intent correctly that this block of code is a primary motivation for the change? Well, along with the related block (new lines 88-90) in the loop where .bind() is called?

That the previous code was relying on a promise chain, and never called .catch() and so one error could stop all the loading?

Yup, you nailed it. The previous recursive calls built up a promise chain where one broken plugin would stop the chain from progressing. It did have some try/catches and some promise.catches at various stages, but they didn't isolate the plugins from breaking each others ability to emit/bind like this should.

dobbs · 2024-07-12T00:05:08Z

lib/plugin.js

-    if (window.plugins[name]) { return resolve(window.plugins[name]); }
-    return getScript(`/plugins/${name}/${name}.js`, function() {
-      let p = window.plugins[name];
+  if (window.plugins[name]) {


This might be something that was already there in the previous code...

There are a lot of guard clauses checking for window.plugins[name] but I can't figure out where that gets assigned. Is that happening in the caller instead of in this file somewhere? (Or am I just missing it?)

I did change this a hair, so it doesn't go into the loading scripts mechanism for plugins that are already there. The default plugins all get added to window.plugins in a separate file https://github.com/fedwiki/wiki-client/blob/master/lib/plugins.coffee#L6-L12, and then non-builtin plugins either call wiki.registerPlugin from plugin.registerPlugin on the last line of this file, or they simply add themselves to window.plugins manually.

So the early checks in this file are looking for plugins that have been previously loaded, and the later checks are testing if a plugin has been successfully loaded by seeing if it added itself to window.plugins, which also means a plugin can't do anything asynchronous before adding itself to the plugins object or this code thinks that it didn't load successfully. (this part isn't a change, but worth looking at after using import to load plugins lands)

dobbs · 2024-07-12T00:09:47Z

lib/plugin.js

+      if (!window.plugins[name]) {
+        await getScript(`/plugins/${name}.js`)
+      }


Here's another case where the async...await is making this backwards compatibility easier to read.

Tho' it's another place that makes the window.plugins[name] part conspicuous. In this context it looks like we expect the plugin author to assign to the global plugins and we're checking for that side-effect. (Feeling more convinced this comment is unrelated to this PR, but seems worth thinking about while we're here).

Yeah, that is exactly what has been happening, this change just makes it easier to understand 😅 I don't think there is anything we can do to improve it here yet, but this change combined w/ import to load plugins would make it very easy to add an alternate way to register plugins using es6 module exports.

paul90 · 2024-07-13T10:05:13Z

There probably needs to be some memory that a plugin has not been found. Each load attempt looks in two places, /plugins/chess/chess.js and /plugins/chess.js, and this is done twice. Looks as if the second place goes back to a change in plugin location 12 years ago - see SFW commit.

plugin.get() is used in two? places to load the plugin - line 84, in renderFrom(), and line 267, in plugin.emit(). Once in the render, and then in the bind?

I think much of this is old problems. It has been determined that "Could not find plugin chess", but the code still goes down the path of throwing an error, and repeating the attempt to load the plugin.

paul90 · 2024-07-13T10:54:52Z

Toggling the wiki button, on or off, results in content following the item with the failed plugin to get duplicated. The number of times seems to be dependent on the number of pages in the lineup to the left of the page with the failed plugin! Very strange.

This behaviour previously existed.

nrn · 2024-07-13T14:31:37Z

Yeah, I think those will both be easier fixes after this change, but I was trying not to do too much in the one pr. Trying to load the code multiple times may even just be fixed by import(), but I saw a potential quick fix for it while I was working on the rest of this I'll add as a suggestion.

lib/plugin.js

WardCunningham

Thanks to Nick and Paul for walking me through this step by step.

paul90 · 2024-08-03T13:19:29Z

As mentioned in the call earlier this week - I have created a branch withgit mv plugin.coffee plugin.js as the first step. This helps retain the history of the decaffeinate step. A pre-release is available as wiki-client@0.30.0-rc.1

See https://github.com/fedwiki/wiki-client/commits/bind-error-with-mv/ for the commits in the modified branch with the initial git mv.

paul90 · 2024-08-05T09:37:30Z

The force push was to add git mv as the first commit, and to convince this PR has been merged.

dobbs reviewed Jul 12, 2024

View reviewed changes

nrn commented Jul 13, 2024

View reviewed changes

lib/plugin.js Outdated Show resolved Hide resolved

paul90 mentioned this pull request Jul 15, 2024

Use dynamic import to load plugin client code #320

Merged

This comment was marked as duplicate.

Sign in to view

paul90 self-requested a review July 15, 2024 09:09

WardCunningham approved these changes Jul 30, 2024

View reviewed changes

paul90 approved these changes Aug 5, 2024

View reviewed changes

paul90 closed this Aug 5, 2024

paul90 force-pushed the nrn/bind-error branch from 2e7bfb3 to 0852522 Compare August 5, 2024 09:36

paul90 deleted the nrn/bind-error branch August 14, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert plugin loading to async/await forward JS #321

Convert plugin loading to async/await forward JS #321

nrn commented Jul 11, 2024

dobbs left a comment

dobbs Jul 11, 2024

dobbs Jul 11, 2024

nrn Jul 12, 2024

dobbs Jul 11, 2024

nrn Jul 12, 2024

dobbs Jul 12, 2024

nrn Jul 12, 2024

dobbs Jul 12, 2024

nrn Jul 12, 2024

paul90 commented Jul 13, 2024 •

edited

Loading

paul90 commented Jul 13, 2024

nrn commented Jul 13, 2024

This comment was marked as duplicate.

WardCunningham left a comment

paul90 commented Aug 3, 2024 •

edited

Loading

paul90 commented Aug 5, 2024

Convert plugin loading to async/await forward JS #321

Convert plugin loading to async/await forward JS #321

Conversation

nrn commented Jul 11, 2024

dobbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul90 commented Jul 13, 2024 • edited Loading

paul90 commented Jul 13, 2024

nrn commented Jul 13, 2024

This comment was marked as duplicate.

WardCunningham left a comment

Choose a reason for hiding this comment

paul90 commented Aug 3, 2024 • edited Loading

paul90 commented Aug 5, 2024

paul90 commented Jul 13, 2024 •

edited

Loading

paul90 commented Aug 3, 2024 •

edited

Loading