microsoft / microsoft/node-jsonc-parser
getNodePath/modify do not work for property nodes (capture too much content)
Dieses Issue hat noch niemand übernommen.
- Vorherrschende Sprache
- TypeScript
- Sterne
- 759
- Forks
- 66
- Ø Merge
- 5 T. 10 Std.
- Gemergte PRs (30 T.)
- 7
Beschreibung
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.
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Erste Schritte
- Lies das ganze Issue und danach den Beitragsleitfaden des Projekts.
- Schreib ins Issue, dass du es übernimmst — das erspart doppelte Arbeit.
- Forke das Repository und arbeite in einem Branch.
- Öffne einen Pull Request, der die Issue-Nummer nennt.
Rechercherichtung
Beginne mit der Reproduktion in src/app.ts und führe npm install && npm run build aus, gefolgt vom dokumentierten node-Befehl. Untersuche die Einstiegspunkte getNodePath und modify im Hinblick auf das im Issue beschriebene Verhalten von Property-Pfaden. Als erledigt gilt die Aufgabe, wenn findNodeAtLocation(tree, getNodePath(node)) den Property-Knoten zurückgibt und modify nur diese Property entfernt.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- typescript
- Bereich
- tooling
- Issue-Typ
- Bug
- Schwierigkeit
- 3/5
- Geschätzter Aufwand
- 1-2 Tage
- Aktivitätsstatus
- Veraltet
- Klarheit
- Größtenteils klar
- Anfängerfreundlichkeit
- 42/100