opensearch-project / opensearch-js

Node.js Client for OpenSearch
https://opensearch.org/docs/latest/clients/javascript/
Apache License 2.0
179 stars 118 forks source link

Add new types to package exports definition for ESM support #674

Closed ghost closed 5 months ago

ghost commented 6 months ago

Description

As of today, it's not possible to resolve the new typings with a TS project using ESM. With these changes, you can import types in ESM project, e.g.:

import type { SearchHit, SearchResponse } from '@opensearch-project/opensearch/api/types'

This is also backwards compatible with projects converting from TS + CommonJS to TS + ESM.

Issues Resolved

Closes #204

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

kavilla commented 6 months ago

@AMoo-Miki

dblock commented 6 months ago

Is there a way to add a test for this so we don't break it in the future?

ghost commented 6 months ago

Is there a way to add a test for this so we don't break it in the future?

Unfortunately I'm not sure if there is an easy way to test it - it probably requires setting up a child project with ESM and type checking.

However, after having another look at the type definitions, I noticed the ./api/types -module is already exported from the root typings as opensearchtypes namespace export, similarly as ./api/requestParams. Should we export just the ./api/new as explicit ESM (type) export, and request users to import the types from ./api/types via the root package namespace export? Such as:

import { ApiResponse, opensearchtypes } from '@opensearch-project/opensearch'
import { Client } from '@opensearch-project/opensearch/api/new'

const client: Client
const response: ApiResponse<opensearchtypes.SearchResponse<TDocument>> = await client.search({ ... })

Technically we could export the new types via the root typings as well, but if they're still partially work in progress and opt-in, I'd keep them separate.

ghost commented 6 months ago

@dblock @kavilla @AMoo-Miki not trying to rush anything, but happy to improve upon your request.

I'd suggest we change this PR to export only the new types

    "./api/new": {
      "types": "./api/new.d.ts"
    }

What do you think?

nhtruong commented 6 months ago

@perttukarna Is this related to https://github.com/opensearch-project/opensearch-js/issues/670#issuecomment-1864866676 ?

ghost commented 6 months ago

@nhtruong yes and no. 😄 This won't solve the problem of having two different type definitions for the client, but merely makes the new type definitions available for certain project setups (TS + ESM in particular).

ghost commented 6 months ago

@dblock @kavilla @AMoo-Miki can you take a look, please?

dblock commented 6 months ago

Leaving to @nhtruong to decide and merge.