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

ESModuleのパッケージを読み込めるようにする #2073

Merged
merged 13 commits into from
May 24, 2024

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented May 11, 2024

内容

ESModule製のパッケージを読み込めるようにします。

関連 Issue

(なし)

スクリーンショット・動画など

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner May 11, 2024 12:38
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team May 11, 2024 12:38
@sevenc-nanashi sevenc-nanashi marked this pull request as draft May 11, 2024 13:07
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

実はあまりよくわかってないんですが、ESModuleが良いと思います!
なんか相談したいこととかあれば何でもおっしゃってください!

@sevenc-nanashi
Copy link
Member Author

以下の理由により、"type": "module"をやめました:

  • 制限がややこしい
  • そもそも目的であるESModule製パッケージの読み込みは"moduleResolution": "bundler"と少しの改変でできる

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review May 19, 2024 06:14
@sevenc-nanashi sevenc-nanashi changed the title Migrate: ESModuleに移行 ESModuleのパッケージを読み込めるようにする May 19, 2024
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

いろいろコメント書きましたがほぼLGTMです!

ESModuleやtsconfig.jsonのmoduleResolutionなど、よくわからなかったので調べました。
この記事がとてもわかり易かったです。
https://blog.s2n.tech/articles/dont-use-moduleresolution-node


そもそもなぜESModuleの読み込めるようにしたい感じでしょうか。
(ESModuleのみなパッケージを読めるようにしたいから、とかでしょうか)

あと、拡張子 .js → .cjs の変更は不要になってたりしそうでしょうか?

package.json Show resolved Hide resolved
src/@types/immer.d.ts Outdated Show resolved Hide resolved
src/@types/vuex.d.ts Show resolved Hide resolved
@Hiroshiba
Copy link
Member

あ、あとPRの本文内容コメントも古くなってるかもなので最新のにしていただけると 🙏
(あとから見たときに混乱しないように)

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented May 24, 2024

(ESModuleのみなパッケージを読めるようにしたいから、とかでしょうか)

です。

あと、拡張子 .js → .cjs の変更は不要になってたりしそうでしょうか?

多分不要ですね、元に戻しておきます。
(そのうちここもtsにしたい)

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

手元で試してみて動いたので問題ないと思っているのですが、ちょっと不安なのでwebフロント周りちょっと詳しそうな方にメンションさせていただきます・・・!
何か問題点とか思い当たったらコメントいただけると・・・ 🙇
@yamachu @MT224244 @Segu-g

そのうちここもtsにしたい

確かに!!! その発想がなかったです。。
(この辺りの実行だけDenoにするのもありだと思います。なんか面倒事が発生しない限り。)

@Hiroshiba Hiroshiba merged commit 29195c3 into VOICEVOX:main May 24, 2024
9 checks passed
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.

2 participants