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

[PROPOSAL] [CCI] TypeScript client rewrite. Help wanted #444

Open
timursaurus opened this issue Mar 18, 2023 · 7 comments
Open

[PROPOSAL] [CCI] TypeScript client rewrite. Help wanted #444

timursaurus opened this issue Mar 18, 2023 · 7 comments
Assignees
Labels
CCI College Contributor Initiative

Comments

@timursaurus
Copy link
Collaborator

timursaurus commented Mar 18, 2023

What/Why

What are you proposing?

Rewriting the while client from JavaScript to TypeScript, including the test suite.
Additionally, improving (IMO.) the folder structure of the project

Everything is moving to the src directory.
📁 src
|--📁 api
|----📁 internal <-- better than api/api
|------bulk.ts
|------cat.ts
|------...
|---- class OpenSearchAPI
|
|--📁 transport
|----📁 aws
|----📁 connection
|----📁 pool
|----Serializer.ts
|----...
|--client.ts

If using any Node modules, the node: prefix will be used, example:

import assert from 'node:assert';
import http from 'node:http';
import https from 'node:https';
import { pipeline } from 'node:stream';
import { inspect } from 'node:util';
import type { ConnectionOptions as TlsConnectionOptions } from 'node:tls';

Module path resolving for better Developer Experience (Contributor):

import { ResponseError, ConfigurationError } from '@/errors';  // <-- instead of './../../errors'
import { NOOP } from '@/utils';

(Opinionated)
Going further, we can also use custom modules names that lead to certain directories, for easier access, and it marks that the module is internal.

import { Connection } from '#transport';
import { BulkApi } from '#internal'

The work has been started on my fork-repo (ts branch): https://github.com/timursaurus/opensearch-js/tree/ts
At the moment, 3 people are interested in helping to rewrite the client and contacted me

If you're interested in helping, contact me through Telegram (@timursaurus ) or Github. Any help works :)
Or if you have any problems making the PR, or don't know how it works and what to do, I'll guide you through

What users have asked for this feature?

#419 (comment)
#269
#266 (comment)
#266 (comment)

What problems are you trying to solve?

Improving the developer experience, making the development less error prone, avoiding obvious bugs and misalignment between JSDoc and the Client itself, like:
#430
#443 - This has been found recently, when converting the client to TypeScript. Something that's very hard to notice, but type-checking prevents such errors. We made the PR immediately :)
#421

Are there any breaking changes to the API

No.

Are there breaking changes to the User Experience?

Apart from the better Text Editor Intellisense, no.

What will it take to execute?

Time, experience and someone knowledgeable who knows all the quirks of the client

Any remaining open questions?

Help wanted!

@timursaurus
Copy link
Collaborator Author

timursaurus commented Mar 18, 2023

At the moment, I'm working on the 📁 transport and 📁 api > OpenSearchAPI class dirs.
In the OpenSearchAPI class, I'm working on ditching the prototype pattern to using static properties and getters

Util method like NOOP will be only reused, not declared in each file.
Serializer has been been fully converted to TS. Helpers is on the way

TypeScript Compiler tsc will be used to build the project.
Different tsconfigs will be needed for mjs and cjs outputs

@nhtruong nhtruong added CCI College Contributor Initiative and removed untriaged labels Mar 21, 2023
@timursaurus
Copy link
Collaborator Author

timursaurus commented Mar 22, 2023

What's been done so far

📁 src
|--📁 api
|---L📁 internal
|-----L...
|---- class OpenSearchAPI
|
|--📁 transport
|---L📁 aws
|---L📁 pool
|-----LBaseConnectionPool.ts - WIP
|-----LCloudConnectionPool.ts
|-----LConnectionPool.ts
|----Serializer.ts
|---- Helpers.ts - WIP
|----...
|--client.ts - WIP
|--errors.ts
|--utils

@dblock
Copy link
Member

dblock commented Mar 22, 2023

@timursaurus you'll be my hero if you can pull this off!

@timursaurus
Copy link
Collaborator Author

timursaurus commented Mar 26, 2023

The development has been moved to a new repository: https://github.com/timursaurus/opensearch-ts/tree/dev

OpenSearchAPI class methods will be implemented only using classes and inheritance
Example:

export class BulkImpl {
  constructor(protected transport: Transport) {
    this.transport = transport;
  }
  /**
   * The bulk operation lets you add, update, or delete many documents in a single request.
   * Compared to individual OpenSearch indexing requests, the bulk operation has significant performance benefits.
   * Whenever practical, we recommend batching indexing operations into bulk requests.
   * <br/> See Also: {@link https://opensearch.org/docs/latest/api-reference/document-apis/bulk/|OpenSearch - Bulk}
   *
   * @memberOf API-Document
   * @param params
   * @param options
   * @param callback
   */

  //... (overloads)
  bulk<TSource = unknown, TContext = unknown>(
    params: BulkRequest<TSource>,
    options: TransportRequestOptions,
    callback: CallbackFn<BulkResponse, TContext>
  ): TransportRequestCallback {
    // implementation
    return this.transport(params, options, callback)
  }
}

We use protected so that we cannot access transport

client.cat.transport 🛑 // <-- not accessible 

client.cat.health(...) 
client.cat.tasks(...) 
client.bulk(...) 
export class OpenSearchAPI {
  cat: CatImpl
  bulk: BulkImpl['bulk']
  ping: PingImpl['ping']
  constructor (options: ClientOptions) {
    this.cat = new CatImpl(options.Transport)
    this.bulk = new BulkImpl(options.Transport).bulk
    this.ping = new PingImpl(options.Transport).ping
  }
}

vitest will be used instead of tap and tsd

@zloiadil
Copy link

I want to join this issue @timursaurus

@sayuree
Copy link
Contributor

sayuree commented Mar 30, 2023

I want to help with this issue

@AbhinavGarg90
Copy link

I'd love to know how I can get started working on this @timursaurus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCI College Contributor Initiative
Projects
None yet
Development

No branches or pull requests

6 participants