Convert plugin loading to async/await forward JS #321
Conversation
lib/plugin.js
Outdated
| if (itemElems.length === 0) { return promise; } | ||
| const itemElem = itemElems.shift(); | ||
|
|
||
| for (const itemElem of $items.toArray()) { |
There was a problem hiding this comment.
noticing out loud that I like the change from a recursive call to emitNextItem() to a for...of loop.
lib/plugin.js
Outdated
| $item.off(); | ||
| return plugin.emit($item.empty(), item, () => resolve()); | ||
| })); | ||
| await plugin.emit($item.empty(), item) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
lib/plugin.js
Outdated
| } catch (e) { | ||
| console.error(e) | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
lib/plugin.js
Outdated
| 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]) { |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)
lib/plugin.js
Outdated
| if (!window.plugins[name]) { | ||
| await getScript(`/plugins/${name}.js`) | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
There probably needs to be some memory that a plugin has not been found. Each load attempt looks in two places,
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. |
|
Toggling the 
This behaviour previously existed. |
|
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 |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
As mentioned in the call earlier this week - I have created a branch with See https://github.com/fedwiki/wiki-client/commits/bind-error-with-mv/ for the commits in the modified branch with the initial |
|
The force push was to add |
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.