Async API changes may break plugins

[raybellis]

Most of the exported functions from the Etherpad internals will no longer take callback parameters in Etherpad 1.8+ (see #3540) and return Promises instead. This may break plugins that rely on those functions.

If backwards compatibility is required one solution may be to use the nodeify module to conditionally wrap the invocations of those exported functions.

[raybellis]

Here's an example of how an existing plugin has been modified to provide backwards compatibility:

https://github.com/raybellis/ep_small_list

and also an example of how that same plugin would now be written if backwards compatibility is not required:

https://gist.github.com/raybellis/0a99d4762364605223bb79f84e4d59cf

NB: I would expect to be able to write a cleaner method to achieve the backward compatibility such that if multiple functions are to be wrapped it wouldn't take that amount of code for each function, perhaps even adding a module to the core distribution to handle the bulk of it for them.

[raybellis]

We should probably add some sort of Etherpad API versioning to the internal modules (collectively, not per module) so that the /admin page can indicate which plugins are compatible.

[Wikinaut]

@raybellis Just as an idea: perhaps the compatibility can programmatically be tested at runtime, perhaps at least in some cases.

[raybellis]

@Wikinaut see my forked version of your plugin, which does just that.

[Wikinaut]

@raybellis In general I meant above: Etherpad core (admin module) can perhaps test (with assertions?) whether or not the installed plugins are async-compatible.

[raybellis]

Oh, I see - the other way around? That's kind of what I meant by a versioning system, i.e. an expectation that each plugin's ep.json file specifies its minimum required Etherpad version. It would be very difficult to actually programmatically figure out whether a plugin is using callbacks or not.

[raybellis]

I've made good progress writing some additional code that adds callback compatibility back into all of the exported functions that are now async, such that the ep_small_list plugin now works again without modification.

The code is slightly hacky, insomuch as it uses eval to create a new wrapper function that has the same arity as the existing functions. It's committed to my async branch (commit 64fdc73) so that it can be cherry picked if you want it.

Further testing is however stalled due to #3567.

[Wikinaut]

@raybellis I confirm that also my installation (currently 64fdc73 ) is now working with the original ep_small_list plugin. Thank you very much.

[muxator]

Hi, I still did not integrate this into async-PR.

[muxator]

Hi, I am going to merge #3559 now. It contains all of your work on the async branch, except for your proposed fix for this issue (64fdc73), to give us more time to think about a solution.

If we'll continue to like that approach, please remember me to cherry pick this change in the next days.

[raybellis]

The eval hack is only necessary to preserve the arity of the original functions, and was added to allow clients of the API elect to probe the arity to check whether the API is callback or promise -based (per my first attempt to show how to support either model).

Since the patch adds back in the ability to use either method based on the arguments received the support for arity testing could be dropped, and the wrappers generated as simple closures.

[muxator]

I have lost context about this issue, but I am going to release 1.8.0 anyway.
Moving to 1.8.1 to keep track of it.

[mspae]

I've cloned two existing but apparently not maintained plugins and refactored them to work with the async functions:

• https://gitlab.com/gebrauchsgrafik/ep_httpauth_author (original: ep_auth_author)
• https://gitlab.com/gebrauchsgrafik/ep_pad_overview (original plugin: ep_pad-lister)

Note that I provide no further assurances that these are in any way safe or usable.

BTW awesome that you refactored this @raybellis – the plugin code is much nicer to read now it uses async/await.

I think once this is released as a proper release there would need to be a big notice saying that there are severe breaking changes in the plugin API I think otherwise people might be confused. Also from a strict semver perspective: Would the proper release version not be 2.0.0 since there are breaking changes? – I don't know if this project follows this spec. There are certainly other options.

[JohnMcLear]

Hey guys, I already refactored most of my plugins so they should work. Is there a reason this issue is still open?

[muxator]

We can close this issue, now that the first point release (1.8.3) after the async rewrite (1.8.0) is ready to be released, and the few actively maintained plugins have been adapted.

Thanks.