fix(use-case/nodecli): ESModule対応 #1465
Conversation
✅ Deploy Preview for js-primer ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| const sample = fs.readFileSync(path.resolve(__dirname, "./fixtures/sample.md"), { encoding: "utf8" }); | ||
| const expected = fs.readFileSync(path.resolve(__dirname, "./fixtures/expected.html"), { encoding: "utf8" }); | ||
| const sample = fs.readFileSync("test/fixtures/sample.md", { encoding: "utf8" }); | ||
| const expected = fs.readFileSync("test/fixtures/expected.html", { encoding: "utf8" }); |
There was a problem hiding this comment.
import.meta自体の説明はあってもいい気はするけど、__dirname はちょっと遠いから迷うなー
import url from "node:url";
import path from "node:path";
const __filename__ = url.fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename__);new URL('fixtures/sample.md', import.meta.url);ならもうちょっとスッキリする?
import.meta.resolve も将来あるし、迷いどころ。
Node.js の import.meta.resolve について
とりあえずはこのままでもいいけど、これってcwdからの位置になるんだっけ?
まあ、npm testで実行にしてるならずれないか
There was a problem hiding this comment.
cwdからの位置ですね。npm test前提なのでこれでよしとしたいです
| 同期APIと非同期APIはどちらも`fs`モジュールに含まれていますが、 | ||
| 非同期形式のAPIは`fs/promises`というモジュール名でも参照できるようになっています。 | ||
| この書籍では分かりやすさのために、非同期形式のみのAPIを提供する`fs/promises`モジュールを利用します。 | ||
|
|
||
| Node.jsの標準モジュールは`node:fs`のように`node:`プレフィックスをつけてインポートできます。 |
There was a problem hiding this comment.
source/use-case/nodecli/refactor-and-unittest/src/test/md2html-test.js
Outdated
Show resolved
Hide resolved
|
test落ちてるのは #1464 で、ESMのexample testができなくて、cjsとesmでテスト方法を分けたからっぽい
|
Co-authored-by: azu <[email protected]>
|
テストが落ちてるのはtodoapp側っぽい？？よくわかってない |
test/example-es-test.js
Outdated
| @@ -14,6 +14,7 @@ describe("example:es", function() { | |||
| const esmFiles = globby.sync([ | |||
| `${sourceDir}/use-case/todoapp/**/*-example.js`, // *-example.js | |||
| `${sourceDir}/use-case/todoapp/**/*.example.js`, // *.example.js | |||
| `${sourceDir}/use-case/nodecli/**/*.js`, // *.js | |||
There was a problem hiding this comment.
これだとexample以外も読み込んで落ちてるんじゃないかな?
${sourceDir}/use-case/nodecli/**/example/*.js が正しそう
There was a problem hiding this comment.
greet.jsとgree-main.jsぐらいがテストされるはず。
md2html.jsとか読み込んでもエラーしそうな気もする
|
テスト通るようになったらlint errorちゃんと出るようになったんで修正追いかけましたが、もう再レビュー可能そうです |
There was a problem hiding this comment.
細かいところコメントした。
LGTM
細かいけど
https://github.com/asciidwango/js-primer/pull/1465/files#diff-00a68ff81c59d6d9c766171da538662ae315ad602b2149617acd993733195d4dR95
test/md2html-test.js だけファイル名のtitleがついてないことに気づいた。
| 次の例では、先ほどインストールした`commander`パッケージを`node_modules`ディレクトリから読み込んでいます。 | ||
| Node.jsはECMAScriptモジュールからCommonJSモジュールをインポートする方向の相互運用性をサポートしています。 | ||
| たとえば、次のようにCommonJSモジュールで`exports`オブジェクトを使ってエクスポートされたオブジェクトは、ECMAScriptモジュールで`import`文を使ってインポートできます。 | ||
| Node.jsの標準モジュールはECMAScriptモジュールのJavaScriptファイルからでも利用できますが、それはこの相互運用性によるものです。 |
There was a problem hiding this comment.
これあんまり意識してなかったけど、確かにそうなってた。
https://github.com/nodejs/node/tree/main/lib
前提として、Node.jsの標準モジュール自体がCJSで書かれてることが抜けている感じがする。
なんか別の例でもいいのかなーとはちょっと思ったけど、思いついてない。
There was a problem hiding this comment.
標準モジュールのことは125行目でさらっと書いてますね
|
マージします |
Close #1355
要確認
__dirnameが使えなくなったのをどうするかなーと思ったが、import.metaの説明増やすのもアレなのでpath.resolveすること自体やめて（もともと説明してなかった）、package.jsonからの相対パスで読み込むようにしたが、問題あります？