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

fix: read correct type definitions when using ESM #87

Merged
merged 1 commit into from
Mar 31, 2024

Conversation

sun-yryr
Copy link
Contributor

fixed #86

Summary

Modified package.json to use the appropriate type definition files based on CJS/ESM.
The necessary files have already been output by pnpm build.

Changes

Modified package.json to use the appropriate type definition files based on CJS/ESM.

Output Directories (pnpm build)

dist
├── index.cjs
├── index.d.cts
├── index.d.mts
├── index.d.ts
└── index.mjs
Japanese

概要

package.json を修正し、CJS/ESM に応じた型定義ファイルを使用するようにしました。
必要なファイルはすでに pnpm build で出力されています。

変更内容

CJS/ESM に応じた型定義ファイルを使用する形に package.json を修正

出力されたディレクトリ (pnpm build)

dist
├── index.cjs
├── index.d.cts
├── index.d.mts
├── index.d.ts
└── index.mjs

@yamatatsu
Copy link
Member

@sun-yryr
Thank you for creating this issue and the PR!
It was reproducted with the way as you described.
And I have a question how your fixing work.
In my confirmation, the change does not seems to make any differnce to dist.
If you resolved this issue with your way, please tell me the differnce of dist/index.d.mts created by pnpm build. 🙇

And, a way I found (but not smart) is adding named export to index.ts as following;

import { AthenaQuery } from "./athena-query";

export default AthenaQuery;

// Add this line
export { AthenaQuery } from "./athena-query";

And users will use it when using ESM;

import { AthenaQuery } from "@classmethod/athena-query";

const athenaQuery = new AthenaQuery(athena);

It will work without any breaking changes, but cannot provide default exported class.😕

Japanese

このIssueとPRを作成していただきありがとうございます!
説明いただいた方法で再現できました。
そして、あなたの修正がどのように機能するか質問があります。
私が確認する限りでは、この変更ではdistに何の変化もないようです。
もし違いがある場合はどのようにしているか教えてください...

そして、私が見つけた方法(賢くないが)は、index.tsに名前付きエクスポートを追加することです。

import { AthenaQuery } from "./athena-query";

export default AthenaQuery;

// Add this line
export { AthenaQuery } from "./athena-query";

そして、ユーザーはESMを使用するときにそれを使用します。

import { AthenaQuery } from "@classmethod/athena-query";

const athenaQuery = new AthenaQuery(athena);

これは、破壊的な変更なしに機能しますが、デフォルトエクスポートされたクラスを提供することはできません。😕

@sun-yryr
Copy link
Contributor Author

@yamatatsu
Thank you for reviewing the PR!
The generation of .d.mts and .d.cts files was added in [email protected]. By updating the dependencies, these type definition files will be generated.
It seems that the dependencies have already been updated, so my understanding is that you just need to run the build.
Please let me know if there's anything else I can assist with.

Japanese

PR の確認ありがとうございます!
.d.mts, .d.cts の生成は [email protected] で追加されました。依存を更新することでこれらの型定義ファイルが生成されます。
依存の更新は既に行われているようですので、ただビルドするだけで良い認識です。


I have run the following commands locally and confirmed the contents of the dist directory:

$ gh repo clone sun-yryr/athena-query
$ cd athena-query
$ git switch fix/#86-type-error
$ pnpm install
$ pnpm run build
$ ls -1 dist
index.cjs
index.d.cts
index.d.mts
index.d.ts
index.mjs

# versions
$ pnpm --version
8.6.5
$ pnpm why unbuild
Legend: production dependency, optional only, dev only

@classmethod/[email protected] /Users/***/athena-query

devDependencies:
unbuild 2.0.0

@yamatatsu
Copy link
Member

@sun-yryr
To confirm this change, I tried to do following operation after modifing package.json as your PR.

mv dist dist_
pnpm build
diff dist dist_

But no difference was found.🤔 It means the 5 files had been created with the package.json before modified.
How is it on your environment?

@yamatatsu
Copy link
Member

And you can see the files on the repository npm hosted.
https://www.npmjs.com/package/@classmethod/athena-query?activeTab=code

@yamatatsu
Copy link
Member

Actually I'm not familiar to unbuild...😅 But Same code as the code before modified is written in official example.
https://github.com/unjs/template/blob/main/package.json#L10-L14

In Japanese;
unbuild慣れてなくて、よくわかってないではあるのですが、一応公式サンプルにもこの書き方が書いてありました!(多分これをコピペしたと思います。。)
https://github.com/unjs/template/blob/main/package.json#L10-L14

@sun-yryr
Copy link
Contributor Author

@yamatatsu
This PR is not involved in the generation of the 5 files. That is a feature of unbuild, and the generation of those files was enabled by the update in 65142e2. (So in v1.3.0, the 5 files were generated.)
This PR simply specifies the paths so that ESM can load the correct type definition files. It does not make any changes to the dist directory.

@yamatatsu
Copy link
Member

@sun-yryr
Wooow! I got my wrong and I've understood this change.🤩
I represented and fixed it with pnpm patch.
https://github.com/yamatatsu/athena-query-issues-86/blob/main/package.json#L19-L23

Thank you to your super kindness explanation!!😭
まじでありがとうございます!!!!

Copy link
Member

@yamatatsu yamatatsu left a comment

Choose a reason for hiding this comment

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

LGTM!

@yamatatsu yamatatsu merged commit 309095b into classmethod:main Mar 31, 2024
1 check passed
@sun-yryr sun-yryr deleted the fix/#86-type-error branch March 31, 2024 15:49
@yamatatsu
Copy link
Member

Released as v1.4.0. https://www.npmjs.com/package/@classmethod/athena-query
Thank you!!!
🎉 🎉 🎉

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.

Type error when using "moduleResolution": "Node16" and ESM
2 participants