getNodePath/modify do not work for property nodes (capture too much content)
Nobody has claimed this yet.
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 42/100
- Issue type
- Bug
- Clarity
- Mostly clear
- Activity status
- Stale
- Tech stack
- typescript
- Domain
- tooling
Research direction
Start with the reproduction in src/app.ts and run npm install && npm run build followed by the documented node command. Inspect the getNodePath and modify entry points against the property-path behavior described in the issue. Done means findNodeAtLocation(tree, getNodePath(node)) returns the property node and modify removes only that property.
Written by the indexing model from the issue text.
Description
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 fromnode.- 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:
- The path printed is
[ 'levels', 0, 'screen' ]. In other words, as printed, it points to the enclosing dictionary, not thetransformBackgroundproperty. - 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".
- 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.
- Dominant language
- TypeScript
- Stars
- 759
- Forks
- 66
- Avg merge
- 5d 10h
- Merged PRs (30d)
- 7
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from microsoft/node-jsonc-parser
-
Difficulty 3/5 1-2 days Newbie friendliness 72/100
microsoft/node-jsonc-parser#125 · 1 comment ·
-
Difficulty 3/5 1-2 days Newbie friendliness 45/100
microsoft/node-jsonc-parser#105 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
microsoft/node-jsonc-parser#103 · 2 comments · 8 reactions ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 48/100
microsoft/node-jsonc-parser#97 · 2 reactions ·
-
Difficulty 4/5 3-5 days Newbie friendliness 35/100
microsoft/node-jsonc-parser#95 · 1 reaction ·
All issues in microsoft/node-jsonc-parser
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
Eynzof/Hermes-CN-Desktop#610 ·
-
bug clawsweeper:linked-pr-open clawsweeper:needs-live-repro clawsweeper:no-new-fix-pr impact:message-loss issue-rating: 🐚 platinum hermit P2 regression
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
-
enhancement
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
-
calcite-components needs triage refactor
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
Esri/calcite-design-system#15203 ·
-
Difficulty 1/5 Under an hour Newbie friendliness 78/100
fullcalendar/fullcalendar#8106 ·