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

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

Merged
merged 1 commit into from Jan 8, 2024
Merged

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

merged 1 commit into from Jan 8, 2024

Conversation

ghost
Copy link

@ghost ghost commented Dec 18, 2023

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

  • New functionality includes testing.
    • All tests pass
  • Linter check was successfull - yarn run lint doesn't show any errors
  • Commits are signed per the DCO using --signoff
  • Changelog was updated.

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.

@ghost ghost marked this pull request as ready for review December 18, 2023 09:22
@kavilla
Copy link
Member

kavilla commented Dec 18, 2023

@AMoo-Miki

@dblock
Copy link
Member

dblock commented Dec 18, 2023

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

@ghost
Copy link
Author

ghost commented Dec 19, 2023

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
Copy link
Author

ghost commented Dec 20, 2023

@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
Copy link
Collaborator

@perttukarna Is this related to #670 (comment) ?

@ghost
Copy link
Author

ghost commented Dec 21, 2023

@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
Copy link
Author

ghost commented Jan 4, 2024

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

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAUK this looks correct, but 🤷‍♂️

@dblock
Copy link
Member

dblock commented Jan 4, 2024

Leaving to @nhtruong to decide and merge.

@nhtruong nhtruong merged commit 517779b into opensearch-project:main Jan 8, 2024
@ghost ghost deleted the fix/import-new-types-esm branch January 9, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect types in package.json to api/new.d.ts
3 participants