Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Morebits.wiki.page: overhaul callback arguments and enable use of promises #783

Open
siddharthvp opened this issue Dec 10, 2019 · 1 comment
Labels
Module: morebits The morebits.js library

Comments

@siddharthvp
Copy link
Member

siddharthvp commented Dec 10, 2019

Arguments made available to callback functions in Morebits.wiki.page at present are pathetically inconsistent (noted here by @atlight). The argument passed can be the the page object (Morebits.wiki.page) or api object (Morebits.wiki.api) depending on what function it is and whether it's the success or failure callback. In some cases, the ctxobject (which is supposed to be private) itself becomes available as the this reference in the callback!

For the many cases where the page object is passed to the fail callback, it is not possible for client code to get to know what the error is (the errorCode being in the api object) - which makes it impossible to do anything on the basis of the type of error (such as show a customised error message or do a retry. There are usecases within Twinkle for this - such as #176).

My plan to clean up the mess is:

  • Never pass the api object to any callback or as this. This is in line with Morebits.wiki.page implementation strategy of keeping private stuff private. This allows us to make internal changes to
    the class without fear of breaking existing code that could be making use of internal variables.
  • Rather, give the errorcode and the errortext as the 2nd, 3rd arguments, in addition to the page object which is always the first argument.

I earlier did some work in fixing this in #679 - the approach there being simply to try give the api object every time. I now think that this is the wrong way to go because (i) api object is supposed to be private, (ii) sometimes errors occur before we use any API call - such as when a non-admin tries to protect, or edit summary is not set before a save. In such cases, there is no api object to pass. Passing the page object every time - for success and all types of failures - gives a clean and consistent interface. Where no API call is made before erroring out, we can make up our own error code.

It makes sense to deal with this at the same time as converting Morebits.wiki.page to use promises, because both require painful testing (especially in light of the incredibly unpredictable nature of this in javascript) I will be writing tests along the way. Outside Morebits, in the modules, there is very little that would need to be changed, because use of callbacks will still be supported alongside promises, and callback argument changes would require only require minimal tweaks here and there.

I already have some work on this (example commit), and plan to complete it and open a PR after #765 is merged. Opening the issue now in case people want to discuss the callback overhaul stuff - cc @atlight @MusikAnimal @Amorymeltzer.

@siddharthvp
Copy link
Member Author

siddharthvp commented Feb 5, 2020

Rather, give the errorcode and the errortext as the 2nd, 3rd arguments, in addition to the page object which is always the first argument.

Fiddling with this further, I think giving the errorCode as the 2nd arg may not be the best idea. That would mean that the first arg (pageobj) would be unused in the error handling function most of the time (where only the errorCode is needed). It may be better to put the errorCode and errorText as properties on pageobj and give the pageobj as the sole argument to failure handlers. Edited: While this would be an alternative, it raises potential issues if the same page object is used for multiple action (the error props on the object may be from a previous action). So both methods have a minor drawback - I think it becomes a matter of choice - I'd rather stick with the original idea to avoid re-doing the done work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: morebits The morebits.js library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants