getNodePath/modify do not work for property nodes (capture too much content)

オープン
#51 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
3/5
見積もり時間
1〜2日
初心者へのやさしさ
42/100
issue の種類
バグ
明瞭さ
おおむね明確
活発さ
停滞
技術スタック
typescript
領域
tooling

調査の方向性

src/app.ts で再現から始め、npm install && npm run build を実行してから、ドキュメントに記載された node コマンドを実行します。Issue に記載されているプロパティパスの動作に照らして、getNodePath と modify のエントリポイントを調査します。findNodeAtLocation(tree, getNodePath(node)) がプロパティノードを返し、modify がそのプロパティだけを削除すれば完了です。

索引モデルが issue の本文から書いたものです。

説明

When run on a property node, getNodePath appears to instead returns a path to the enclosing dictionary of the property. This results in some serious problems:

  • findNodeAtLocation(tree, getNodePath(node)) is a different node from node.
  • Calling modify() on a property node removes additional content other than the property node targeted.

Repro steps

Check out the jsonc-parser-demo branch of this github repo.

Run these steps:

npm install && npm run build
node . -d transformBackground testcase.json > testcase-post.json

src/app.ts in this repro is a short script that processes a json file and removes all instances of the property whose key name is given as the -d argument, then prints the file out to STDOUT. To demonstrate the bug more clearly, it currently removes only the first instance before bailing out, and once it has identified the property to be removed it console.warns to STDERR (1) the node it intends to remove (2) the getNodePath of the target node and (3) the roundtrip result of findNodeAtLocation(tree, getNodePath(node)) on the target node. The important code in this example is the code immediately following the comment "PERFORM OPERATIONS HERE"; everything else only serves to load the file and walk the node tree.

My expected behavior running this test script on the included testcase JSON is that on STDERR the "DELETING" and "BUT REALLY DELETING" nodes will be the same (ie, findNodeAtLocation(tree, getNodePath(node)) should be a noop); and on STDOUT the single property "transformBackground" in the dictionary levels/0/screen will be removed.

My observed behavior is quite different:

  1. The path printed is [ 'levels', 0, 'screen' ]. In other words, as printed, it points to the enclosing dictionary, not the transformBackground property.
  2. The "DELETING" prints as expected the property I'm looking for, but the "BUT REALLY DELETING" is different and contains much more content— it does appear to encompass the entire dictionary stored under "screen".
  3. After applying the edits, jsondiff shows this change for the output file: [{"path": "/levels/0", "value": {"color": [0.3, 0.37, 1], "png": "level/test/trigger/trigger.png"}, "op": "replace"}]. In other words, it shows the "screen" property and its value were deleted from /levels/0.

Analysis

I am able to get the "expected behavior" I desire by, instead of passing at.node to getNodePath in the "PERFORM OPERATIONS HERE" section, passing in either of its children. If I do this I get the path [ 'levels', 0, 'screen', 'transformBackground' ] as expected and this is indeed the content that gets removed. However this feels wierd, and additionally it contradicts the documentation. The comment/doc for modify says: " * @param path The path of the value to change. The path represents either to the document root, a property or an array item." This implies modify should accept property nodes.

主要言語
TypeScript
スター
759
フォーク
66
平均マージ
5日 10時間
マージ済み PR(30日)
7

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

microsoft/node-jsonc-parser のほかの issue

microsoft/node-jsonc-parser の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。