Change 233635 had a related patch set uploaded (by Jdlrobson):
Hygiene: Use core code where available for api

https://gerrit.wikimedia.org/r/233635

I've been thinking about 233635 a lot and this is where I'm at with it:

Both the Api and ForeignApi classes specialise the behaviour of their core counterparts (mw.Api and mw.ForeignApi), so it's reasonable to extend them. However, I really dislike the way we're bending over backwards to extend them using our non-standard framework… thing, when what we should be doing is wrapping them in a gateway. By doing so we retain full control of the API that we want to use throughout our codebase – the core API might change but we'd only have to update one class and not the rest of our codebase – at the cost of a little more code.

As a f'rinstance, here's what I think our Api class should look like:

var Api = Class.extend( {
  initialize: function ( coreApi ) {
    this.coreApi = coreApi;
  },

  ajax: function ( /* ... */ ) {
    // Additional magic

    this.coreApi.ajax( /* ... */ );

    // More additional magic
  }
} );

We'd also need to update any code that instantiates the Api class, but I think that's only the line that creates the singleton instance (right?).

There's also the matter of the all of the instances of Api.extend (or ForeignApi.extend sometimes). I've reviewed the NearbyApi and PhotoListApi classes – I might have stumbled upon the only two pieces of code that help me make my point, but I'm confident that isn't the case – and I really don't think that they specialisations in the OO sense. Instead, they represent ways of interacting with external systems – though, at the time of writing, all of our external systems are exposed via our API. The NearbyApi and PhotoListApi are also gateways, which follow the very formulaic pattern of:

1. Make an API call
   1. If the API call fails, then pass the failure on
   2. If the API call succeeds, then convert the response into a collection of entities

i.e.

var NearbyPagesGateway = Class.extend( {
  initialize: function( api ) {
    this.api = api;
  },

  getNearbyPages: function () {
    this.api.ajax( /* ... */ )
      .then( function ( response ) {
        // Convert the response into a collection of entities.
      } );
  }
} );

What's neat is that if we make the dependence on the API explicit in our code, i.e. we inject an instance of the Api class at construction time, then our code is more flexible – the ForeignApi class shares the same interface as the Api class, thus our class is agnostic to where the API is – and more easily tested in isolation.

What you are describing is a significant refactor but I'll give it ago. I fear there might be a bit of code duplication and a chain of patches to get there but we'll see...

@phuedx - attempted for the two modules you pointed out
Considering the work involved in https://gerrit.wikimedia.org/r/237279 I'd like to do this iteratively and slowly and work off the patch you have the issue with.

I will commit to doing this if you can commit to such an approach. This should eventually result in us not having Api.js or ForeignApi.js in our code which to me is a big bonus.

if you can suggest an easier more efficient way of doing https://gerrit.wikimedia.org/r/237279 please also let me know.

@Jdlrobson: I think we share the same views on where we want to be and how to get work done.

What I'd suggest is the following:

1. We integrate the mw.Api and mw.ForeignApi.core classes as I've suggested in T110718#1619882 while maintaining the Api and ForeignApi interfaces
2. We migrate the subclasses of the Api and ForeignApi classes piecemeal at first and then in bulk once we're more confident with the pattern

If you want to pair on this, then I'm fine with going back and forth via Gerrit or sitting on your shoulder in a Hangout.

Change 233635 abandoned by Jdlrobson:
Hygiene: Use core code where available for api

Reason:
https://gerrit.wikimedia.org/r/237279 < new approach

https://gerrit.wikimedia.org/r/233635

Change 237548 had a related patch set uploaded (by Jdlrobson):
Hygiene: Rewrite EditorApi as gateway

https://gerrit.wikimedia.org/r/237548

Change 237547 had a related patch set uploaded (by Jdlrobson):
Hygiene: Repurpose ImageApi as ImageApiGateway

https://gerrit.wikimedia.org/r/237547

Change 237491 had a related patch set uploaded (by Jdlrobson):
WIP: Hygiene: SearchApi -> SearchApiGateway

https://gerrit.wikimedia.org/r/237491

Change 237279 had a related patch set uploaded (by Jdlrobson):
Hygiene: NearbyApi and PhotoListApi should be gateways

https://gerrit.wikimedia.org/r/237279

Bunch of patches above. WatchListApi, WatchstarApi and PageApi also need doing @phuedx if you want to do the same for those. Once those are turned to gateways we can safely write a patch that removes Api.js from MobileFrontend

There's a little work to be done on 237279 and on 237491.

Change 238610 had a related patch set uploaded (by Jdlrobson):
Hygiene: WatchlistApi -> WatchlistGateway

https://gerrit.wikimedia.org/r/238610

Nearby/PhotoApi: https://gerrit.wikimedia.org/r/237279
Search: https://gerrit.wikimedia.org/r/237491
Editor: https://gerrit.wikimedia.org/r/237548
Watchlist: https://gerrit.wikimedia.org/r/238610

I'm working my way through the backlog of patches.

237279 needs a tiny bit more work and then it's ready to be merged.

TODO

• 237279
• 237491
• 237548
• 238610

Just wasted an hour of my life rebasing after T100467. it was horrible. Please help me wrap this up asap ! :)

Change 237279 merged by jenkins-bot:
Hygiene: NearbyApi and PhotoListApi should be gateways

https://gerrit.wikimedia.org/r/237279

Yeah, that's must've been a tough rebase.

I'm sorry not to have reviewed the three remaining patches sooner @Jdlrobson.

I'd say that there's a small follow up patch after those are merged that converts instances of:

var api = new mw.Api(),
  var gatewayConsumer = new GatewayConsumer( api );

// ...

GatewayConsumer = function ( api ) {
  this.gateway = new Gateway( api );
};

to:

var api = new mw.Api(),
  gateway = new Gateway( api ),
  gatewayConsumer = new GatewayConsumer( gateway );

I'll take on that piece of work if you're a little burnt out.

I'm a little burnt out to be honest. Right now I just want to kill mobile.startup/api.js which kicked off this. Every byte in that startup module counts :)

Change 238610 merged by jenkins-bot:
Hygiene: WatchlistApi -> WatchlistGateway

https://gerrit.wikimedia.org/r/238610

Change 240297 had a related patch set uploaded (by Jdlrobson):
Remove api code that is not used or replicated in core

https://gerrit.wikimedia.org/r/240297

Change 237548 merged by jenkins-bot:
Hygiene: Rewrite EditorApi as gateway

https://gerrit.wikimedia.org/r/237548

Change 237547 merged by jenkins-bot:
Hygiene: Repurpose ImageApi as ImageGateway

https://gerrit.wikimedia.org/r/237547

Discussed with @phuedx we agreed to call this task done when
https://gerrit.wikimedia.org/r/237491 and https://gerrit.wikimedia.org/r/240297 are merged.

We'll create another task to capture the gateway-ification of the other classes.

I submitted a new patch set for 237491 in order to fix some errors/omissions so I'd like @Jdlrobson to take a quick look at it before it gets merged. However, 240297 is C+2.

Looks good to me. Merge at will!

Change 237491 merged by jenkins-bot:
Hygiene: SearchApi -> SearchGateway

https://gerrit.wikimedia.org/r/237491

Change 240297 merged by jenkins-bot:
Remove api code that is not used or replicated in core

https://gerrit.wikimedia.org/r/240297

Setup T113753 and T113752