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(usecase/nodecli): コマンドライン引数のデフォルト値を ?? でセットする #1264

Merged
merged 3 commits into from
May 9, 2021

Conversation

lacolaco
Copy link
Collaborator

spread構文では undefined で上書きしてしまうためnullish coalescingを使ってセットするように修正

Close #1257

@lacolaco lacolaco requested a review from azu September 12, 2020 08:53
@bot-user
Copy link

bot-user commented Sep 12, 2020

Deploy preview for js-primer ready!

Built with commit 67e12ce

https://deploy-preview-1264--js-primer.netlify.app

@lacolaco
Copy link
Collaborator Author

node10と12だとパースできてない?

@azu
Copy link
Collaborator

azu commented Sep 12, 2020

@lacolaco
Copy link
Collaborator Author

mdファイル中のコードブロックはそもそもdisableなので、jsファイル中のほうですね

@dogatana
Copy link
Contributor

@dogatana
Copy link
Contributor

オプションのデフォルトとして false を設定するだけなら commander に任せてしまうのが簡単に思いました。
これだと -h のヘルプメッセージにも値が表示されますし。

program.option("--gfm", "GFMを有効にする", false);

@azu
Copy link
Collaborator

azu commented Sep 13, 2020

Node 14だと、準備の章も変えないといけないですね。
https://jsprimer.net/use-case/setup-local-env/#install-nodejs
14のLTSは2021-10-19からなので、それまで待たないといけないですね。
https://nodejs.org/ja/about/releases/

スナップショットテストで落ちてるので、そこだけNode 14で動かすようにするかとかかな。

オプションのデフォルトとして false を設定するだけなら commander に任せてしまうのが簡単に思いました。

これはもともとオプションオブジェクトのマージの例から来ているんですよね。
https://github.com/asciidwango/js-primer/blob/master/meetings/2016-09-09/README.md#meta%E5%9F%BA%E6%9C%AC%E6%96%87%E6%B3%95-%E3%83%AD%E3%83%BC%E3%83%89%E3%83%9E%E3%83%83%E3%83%97--issue-137

@lacolaco
Copy link
Collaborator Author

そうなんですよねえ...

@azu azu mentioned this pull request Sep 13, 2020
@azu
Copy link
Collaborator

azu commented Sep 13, 2020

cliOptionsのデフォルト値はcommanderに任せる(マージ自体をなくす)というのはありですが、
ここのmd2html.jsにおけるgfmのデフォルト値を Nullish coalescing演算子(??)で書くというのはよくある感じがします。
https://jsprimer.net/use-case/nodecli/refactor-and-unittest/#split-script

こういうイメージ。

module.exports = (markdown, options = {}) => {
    return marked(markdown, {
        gfm: options.gfm ?? false,
    });
};

(この辺の細かいバリエーションは 大量にあるのでちょっとどうするか迷う…)

これはCLIのオプションとNodeモジュールとして使った場合のオプションは別にしたいことがよくあるからですね。
今の実装だと、md2html.js はオプションを全指定必須になっているので、この辺が省略されてる感じはしますね。

とりあえずNode 14は待たないと行けない感じですかね #1246

@azu
Copy link
Collaborator

azu commented Oct 28, 2020

https://nodejs.org/ja/blog/release/v14.15.0/
LTSとなった

@azu
Copy link
Collaborator

azu commented Nov 23, 2020

#1265 でNode.js 14へとアップデートしました

@azu azu force-pushed the fix-nodecli-default-options branch from 59aa2de to 4b3ab99 Compare May 9, 2021 13:14
@azu azu force-pushed the fix-nodecli-default-options branch from 4b3ab99 to 394eb82 Compare May 9, 2021 13:21
@azu
Copy link
Collaborator

azu commented May 9, 2021

この本はNode 14LTS前提なので、Node.js 12のサポートは特に必要ないので、CIから12は外しました

@azu azu merged commit b855ddf into master May 9, 2021
@azu azu deleted the fix-nodecli-default-options branch May 9, 2021 13:44
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.

Node.js で CLI アプリ ー デフォルト設定を定義する
4 participants