Backbase/mosaic-rest-js

API design suggestions

philmander opened this issue · 3 comments

Can you align more with the API proposal I've set out here
https://docs.google.com/a/backbase.com/document/d/19416PpYXYBOAokC6R9YZ4tEZi4GyyFc3LlDST2BNF1E/edit#heading=h.qn96vvqwpf8a

Here are some of my concerns with the current approach:

  • Generally I'd prefer us not to expose the new keyword to API clients. Instead, the root variable exported may act as a factory:
var cxpClient = cxp({
    //...opts
});

and use this to specify the portal end point etc.

  • Mirror the rest api more closely, I think this approach solves alot of problems for you and makes the API more clear for those already familiar with REST API.

For example, you use a boolean parameter to specify is catalog, when instead you can just do:

cxpClient.portals([portalName])
cxpClient.catalog()

Also the term "server" is not something I like in the API naming, I'd rather we extract away that, we just need to be concerned with communicating with a REST endpoint

  • I'm not too keen on exposing the data format as an api call (.xml()), maybe you could specify a default format on client initialization and override this in a more flexible way (e.g .format('xml')). This is future proof against other data transport formats, JSON etc

Thanks for your suggestions Philip!

Library considers portal(not server) as the main target. This is done because 80% of the API calls are targeting portal, not server. This also makes syntax less cluttered as you don't need to type name of the portal for each call.

bbrest.portal().get();

instead of

bbrest.portals('my_portal_name').get();

Because server can have many portals and you are usually working on one(correct me if I am wrong), the logic is to instantiate object for every portal that needs to be targeted.

What is the reason to prefer factory method? Again, I find new keyword cleaner and more descriptive:

var bbrest = new BBRest({...});
var bbrest = BBRest.create({...}); // no AMD module
var bbrest = BBRest({...}); // as AMD module

I agree that targeting catalogs should be done differently. What do you think about adding new methods named serverCatalog and portalCatalog? If I understand well portal catalog is a subset of server catalog, so maybe one method called catalog with flag for portal would be even better?

Regarding the mirroring API conventions, do you think that we should mirror them even if they are less clear? For example:

bbrest.server().method(); // it's clear that I am targeting server
bbrest.portal().method(); // ... and portal

bbrest.portals().method(); // not clear immediately
bbrest.portals('my_portal').method(); // what is targeted?

Why would we remove 'server'? That's what it is, right? It is not only portals.

Regarding the xml() method, I will see if it is possible to remove it completely. It seems to me that it only exists because REST API url doesn't have it's own context like /api but it is mixed with requests that require html as response. So why repeating the mistake? You can see in the code that I am already fixing other API inconsistencies.
If we are sure that new transport formats will be there soon, format('xml') is the right way, I agree. If not and I manage to remove it completely, we can always add it later to keep things as simple as possible.

You can still do:

var retailBanking = bbrest.portals('retail-banking ');
retailBanking.get();
retailBanking.widgets().get()

and this allows you to access mulitple portals from the same api instance

I don't believe its good practice for an API to expose the method of instantiation. i.e. expecting the client to manage instances. Generally, I prefer if this is managed internally and not exposed so the client doesn't have to worry about it.

I don't know why you would want to target the server? This should be abstracted away

Yes, that could work, but then the library will need different design.
Right now, first method always returns new request object.

In your example:

bbrest.portals('retail-banking').get();

first method returns request object, but in

bbrest.portals('retail-banking').widget().get();

first method(portals) returns BBRest instance and second method(widget) returns request object. So it is inconsistent. With different design that could be different.