Types Naming discussion

[jsnoble]

Currently going over the teraslice-client-js package and migrating the types to use the ones listed in the @terascope/types library instead of its own. The last major PR to the types library introduced namespacing of the types so things would not conflict as much with naming of things (( THERE ARE NO BREAKING CHANGES, BOTH WORK))

import { Teraslice } from '@terascope/types';
      
function iterateState(cluster: Teraslice.ClusterState) {
...
}

vs

import { ClusterState } from '@terascope/types';

// is this clusterState from elasticsearch or from teraslice?
function iterateState(cluster: ClusterState) {
...
}

However inside the file we have SOME types for request parameters and pretty much ALL types for responses

import { ApiJobCreateResponse } from '@terascope/types';

// this is confusing so hence the namespace

import { Teraslice } from '@terascope/types';

Teraslice.ApiJobCreateResponse

But I still think this has issues, I propose we adapt a similar way we do things with the elasticsearch response naming

import { Teraslice } from '@terascope/types';


async function createJob(): Promise<Teraslice.Response.JobCreate> {
        ...
}

Its more clear about what this is and its purpose, but its a lot more wordy so there are cons to this

For elasticsearch we have

import { ClientParams, ClientResponse } from '@terascope/types';

  async getMapping(index: string): Promise<ClientResponse.IndicesGetMappingResponse> {
        const params: ClientParams.IndicesGetMappingParams = { index };

        return this.client.indices.getMapping(params);
    }

This is a bit better but its still a little confusing now that the types library contains many params/responses for api endpoints, this might need to be changes to have a namespace for it as well

import { Elasticsearch } from '@terascope/types';
      //or
import { ESSearch } from '@terascope/types';
   // or whatever

  async getMapping(index: string): Promise<Elasticsearch.ClientResponse.IndicesGetMappingResponse> {
        const params: Elasticsearch.ClientParams.IndicesGetMappingParams = { index };

        return this.client.indices.getMapping(params);
    }

[godber]

I think fully namespacing things like Elasticsearch.ClientResponse.IndicesGetMappingResponse makes sense and is pretty much a must. And isn't there import Foo as type syntax that can reduce the need for the long names in scenarios where that is sensible?

[kstaken]

The proposed naming here isn't really consistent between the two scenarios.

Teraslice.Response.JobCreate

vs

Elasticsearch.ClientParams.IndicesGetMappingParams

The first name is good, but to be consistent the second name should be something like:

Elasticsearch.Params.Indices.GetMapping

or

Elasticsearch.Indices.Params.GetMapping

Alternatively, simply dropping the ClientParams namespace also technically cleans it up since the name is already specific enough to disambiguate if you add a module specific top level.

Elasticsearch.IndicesGetMappingParams

And then this name is perfectly consistent and fine.

Teraslice.ApiJobCreateResponse

So to me I think the problem here is really just the use of ClientParams and ClientResponse instead of an Elasticsearch top level name.

Then naming just follows the pattern Module.Type.

[jsnoble]

We have migrated our types with this guidance, closing thread