chbrown / rfc6902

Complete implementation of RFC6902 in TypeScript
https://chbrown.github.io/rfc6902/
322 stars 39 forks source link

replacing root object throws #32

Closed diachedelic closed 6 years ago

diachedelic commented 6 years ago
> applyPatch('a', [{op:'replace',path:'',value:'b'}])
TypeError: Cannot set property 'null' of null
    at replace (/private/tmp/node_modules/rfc6902/patch.js:79:35)
    at /private/tmp/node_modules/rfc6902/index.js:31:16
    at Array.map (<anonymous>)
    at applyPatch (/private/tmp/node_modules/rfc6902/index.js:25:18)
    at repl:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:240:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:441:10)

Other libraries, like fast-json-patch, are happy to apply this patch.

chbrown commented 6 years ago

Given that applyPatch returns a list of operation results / errors (a lamentable API choice, but immutability is not a first-class citizen in JavaScript), I'm not sure how this can be accomplished.

Granted, it should throw a more helpful error, or return an error in the list of results, but variables cannot be assigned by reference in JavaScript.

For instance, an actual example I might try would be:

var username = 'chbrown'
var patch_results = applyPatch(username, [{op: 'replace', path: '', value: 'diachedelic'}])
console.log('username)

Even if applyPatch didn't throw, my mental model for how JavaScript works tells me that username cannot be anything but 'chbrown', since primitive values (like string) are immutable, and I only gave applyPatch a reference to a primitive.

This merits more thinking, because you're right — this is a problem. But with past API design decisions and JavaScript semantics what they are, it's not entirely straightforward.

diachedelic commented 6 years ago

Yes I was thinking the only possible solution is an API change and major version bump. I don't think this is very common usage though.

chbrown commented 6 years ago

Alright, this is now handled (properly? at least, handled like other failed operation), and produces a MissingError result rather than throwing.