Implement applyPatch to fix #147, with all needed specs #167

alshakero · 2017-05-10T09:21:38Z

Fixes: #147

felixfbecker

Thank you for taking this on!

felixfbecker · 2017-05-10T09:45:01Z

src/json-patch-duplex.ts

+   * Apply a single json-patch on an object tree
+   * Returns the result object.
+   */
+  export function applyPatch(tree: any, patch: any, validate = false): any {


Can we default validate to false if typeof validate !== 'boolean'? Otherwise the function doesn't work as a reducer because the index is passed as a third parameter and would be coerced to boolean.

felixfbecker · 2017-05-10T09:45:21Z

src/json-patch-duplex.ts

+    if (patch.path === "") {
+      if (patch.op === 'add' || patch.op === 'replace') {// same for add or replace 
+        // a primitive value, just return it
+        if (typeof patch.value !== 'object') {


|| patch.value === null

felixfbecker · 2017-05-10T09:45:39Z

src/json-patch-duplex.ts

+        // a primitive value, just return it
+        if (typeof patch.value !== 'object') {
+          return patch.value;
+        } else if (typeof patch.value === 'object') {


this condition never evaluates to false as it is checked above already

felixfbecker · 2017-05-10T09:46:01Z

src/json-patch-duplex.ts

+   */
+  export function applyPatch(tree: any, patch: any, validate = false): any {
+    if (validate && typeof tree !== 'object') {
+      throw new TypeError('Tree has to be an object');


why does the document have to be an object?

felixfbecker · 2017-05-10T09:47:59Z

src/json-patch-duplex.ts

+    } else {  // test operation (even if not on root) needs to return a boolean
+      if (patch.op === 'test') {
+        var results = apply(tree, [patch], validate);
+        return results[0];


This breaks the contract of the function to always return the updated document.
If a test operation fails, it should abort the patch by throwing an error.
If you have an array of patches, it should be possible to use array.reduce() to apply all patches. That doesn't work if test returns a boolean, but would correctly abort the patch if it throws an error.

felixfbecker · 2017-05-10T09:50:12Z

src/json-patch-duplex.ts

+      } else { // it's a remove or test on root
+        if (patch.op === 'test') {
+          return _equals(tree, patch.value);
+        } else { // a remove on root


I would check patch.op === 'remove' here and and either error or ignore the patch (return the input) if the operation is anything else. Otherwise any invalid patch would result in a null result and it wouldn't be obvious why.

felixfbecker · 2017-05-10T09:52:21Z

README.md

+
+Applying patches on root is a little tricky, for that you can't replace your root object with an object of a different type in place (eg. can't replace `{item: "item 1"}` with `[{item: "item 1"}]`) on the root level, because the original document is an object `{}` and the new value is an array `[]`, nor can you do the opposite (`[] => {}`), obviously. See https://github.com/Starcounter-Jack/JSON-Patch/issues/147.
+
+However, you can use `applyPatch` method to over come this issue, because it returns the result object, and this allows it to change types.


I think it would be great to document the usage with array.reduce() to apply multiple patches (that may modify the root).

felixfbecker · 2017-05-10T09:54:10Z

src/json-patch-duplex.ts

+        }
+      } else if (patch.op === 'move' || patch.op === 'copy') { // it's a move or copy to root
+        // get the value by json-pointer in `from` field
+        var temp: any = { op: "_get", path: patch.from };


these variables are never reassigned so can use const

felixfbecker · 2017-05-10T09:57:21Z

src/json-patch-duplex.ts

+   * Apply a single json-patch on an object tree
+   * Returns the result object.
+   */
+  export function applyPatch(tree: any, patch: any, validate = false): any {


Signature should be

export function applyPatch<T>(tree: T, patch: Patch<any>): T; export function applyPatch<T>(tree: T, patch: Patch<any>, validate: boolean): T; export function applyPatch<T>(tree: T, patch: Patch<any>, validate = false): T { if (typeof validate !== 'boolean') { validate = false; } }

I'll make it return any for the true that is returned by test op. Agree?

See #167 (comment)

- Enhance typings - Verify if validate is a boolean or default to false - Throw an error when `test` operation fails - Change tests accordingly - Add documentation for using applyPatch with reduce

tomalec · 2017-05-10T17:22:25Z

README.md

+Applies a single `patch` on `obj`.
+
+Returns the modified object.
+


I'd rename it to applyOperationObject/applyOperation to be more consistent with naming in spec. Then I'd remove argument and variable inside to operation

BTW, I'd also adjust the names in apply, to something like:

Applies JSON patch (array of operations) on obj.

I am not 100% satisfied with the name either, but note that the type for a single operation is also already called Patch, while apply() takes an array of that: Patch[]. Renaming those would be a breaking change. It also matches how the other libraries name these types.

Operation could be a bit confusing because there is the op attribute, which is short for operation, that is a string.

In any case I would like to have a short name, applyOperationObject is very long.

@felixfbecker if I understand correctly, he just means the params/args names, it isn't a breaking change.

@tomalec I guess you mean 'rename' instead of 'remove' right?

I know, but imo the name of types and the param names / function name should be consistent. A parameter of type Patch should be named patch, not operation. A function applyOperation should take a parameter of type Operation, not Patch. So I would stay with the "patch" naming meaning a single patch vs calling a single patch "operation".

tomalec · 2017-05-10T17:23:04Z

README.md

+
+Available in *json-patch.js* and *json-patch-duplex.js*
+
+Applies a single `patch` on `obj`.


"Applies single operation object patch on obj"

tomalec · 2017-05-10T17:29:03Z

I'd get deeper into it after our HO meeting, or later this week..

felixfbecker · 2017-05-10T19:15:11Z

src/json-patch-duplex.ts

+          // conflicting types [] vs {} just return the new value
+          if (_isArray(tree) !== _isArray(patch.value)) {
+            return patch.value;
+            //same type, use original apply


This is not a big issue, but why are we making the effort here to mutate the original object at all if the function returns a new reference in some cases anyway? The new object is already in the heap. It is way faster to just switch the reference out than mutating the original object, basically diffing it, removing all properties, then copying over all from the new object. Given the performance focus of this package I would vote for always returning the new value if the root is replaced (it also makes more sense semantically).

I agree I was building a demo and not modifying the original object seems like a semantic must. Let alone the perf boost.

felixfbecker · 2017-05-10T19:16:32Z

src/json-patch-duplex.ts

+          }
+        }
+      }
+    } else {  // test operation (even if not on root) needs to return a boolean


comment is outdated

alshakero · 2017-05-11T09:40:10Z

@felixfbecker I guess we were both off with our definition of patch, and this means even the TS types we merged recently are incorrect. Because the RFC says:

JSON Patch document is a JSON [RFC4627] document that represents an array of objects. Each object represents a single operation to be applied to the target JSON document.

So this:

export interface AddPatch<T> extends PatchBase {
  op: 'add';
  value: T;
}

Isn't a correct interface for a patch 😢 It is correct for operation, though.

felixfbecker · 2017-05-11T09:43:14Z

I am aware and not happy with it either, but again it would be a breaking change to change. What we could do is export the old types as type aliases and deprecate them though.

felixfbecker · 2017-05-11T19:29:51Z

@alshakero @tomalec Wdyt about

naming the function applyOperation
documenting that it applies a single operation and returns the updated document
renaming the types to Operation, AddOperation etc.
exporting the types under an alias as the old name with a deprecation note

alshakero · 2017-05-11T19:37:07Z

@felixfbecker Sounds great. Let's see @tomalec's opinion.

In the meantime, I'll push now some changes I did to applyPatch so you might like to review them. I think it's much faster now.

- Turn to opt-in pure function - Better documentation - Needed tests

felixfbecker · 2017-05-11T19:55:34Z

src/json-patch-duplex.ts

+     */
+  export function applyPatch<T>(tree: T, patch: Patch<any>): T;
+  export function applyPatch<T>(tree: T, patch: Patch<any>, validate: boolean, touchOriginalTree: boolean): T;
+  export function applyPatch<T>(tree: T, patch: Patch<any>, validate = false, touchOriginalTree = true): T {


I am not convinced we should have the third parameter, it bloats the signature and adds more branches to the function (you would have to test this for all operations). JSON Patch's sole purpose is to mutate objects. If you really want to clone the tree on every operation, it can be easily achieved through functional composition:

patches.reduce((obj, op) => applyOperation(deepClone(obj), op), obj)

But this makes the function inconsistently impure. And we have a use case for the pure version already. I don't mind writing tests for more cases.

If you want to keep it, the signature needs to be updated to make touchOriginalTree optional

@felixfbecker doesn't this signature alone suffice?

export function applyPatch<T>(tree: T, patch: Patch<any>, validate: boolean = false, touchOriginalTree: boolean = true): T { if (typeof validate !== 'boolean') { // if validate is not a boolean, it's being called from `reduce` validate = false; touchOriginalTree = true; }

According to TS docs, any param with a default value is optional.

Default-initialized parameters that come after all required parameters are treated as optional, and just like optional parameters, can be omitted when calling their respective function.

No, because that is just the implementation. This signature is overloaded, so the two signatures above the implementation that just end with a semicolon define the signature. Overloading is needed here because without the first signature the function would not work as reducer.

To clarify, the second signature needs to be updated.

export function applyPatch<T>(tree: T, patch: Patch<any>): T; export function applyPatch<T>(tree: T, patch: Patch<any>, validate: boolean, touchOriginalTree?: boolean): T; export function applyPatch<T>(tree: T, patch: Patch<any>, validate = false, touchOriginalTree = true): T { if (typeof validate !== 'boolean') { // if validate is not a boolean, it's being called from `reduce` validate = false; touchOriginalTree = true; } }

This does it, right?

felixfbecker · 2017-05-11T19:56:16Z

src/json-patch-duplex.ts

+      } else if (patch.op === 'move' || patch.op === 'copy') { // it's a move or copy to root
+        // get the value by json-pointer in `from` field
+        const temp: any = { op: "_get", path: patch.from };
+        apply(tree, [temp]);


copy and move should also just return the new value

They do return?

ah sorry, just saw that the apply is only for getting the value 👍

tomalec · 2017-05-12T15:12:19Z

@felixfbecker 👍

naming the function applyOperation

documenting that it applies a single operation and returns the updated document

renaming the types to Operation, AddOperation etc.

exporting the types under an alias as the old name with a deprecation note

I'd just change the docs to match exact RFC naming so "that it applies a single operation object and returns the updated document"

alshakero · 2017-05-15T08:08:56Z

@felixfbecker is there a chance that you do the type aliasing thing? That would be a huge favour because you're the most fluent TypeScripter here.

felixfbecker · 2017-05-15T08:09:37Z

Yeah no problem

Rename Patch to Operation

- Make `applyOperation` standalone (#175) - Deprecate `apply` and throw a warning when called - Fix `_equals` (#166) - Replace parseInt with bitwise opers (#171) - Export `escapePath` and `unEscapePath`(#173) - Add `applyReduer` to use with Array.Reduce - Remove `rootOps` - Add tests for all new functions - Add backwards compability tests - Update readme accordingly

alshakero · 2017-05-18T13:07:15Z

@tomalec Ready for review.

felixfbecker · 2017-05-18T12:56:13Z

src/json-patch.ts

-  export interface PatchBase {
+  export interface OperationResult {
+    result: any,
+    newDocument: any


This should be generic T

felixfbecker · 2017-05-18T12:58:21Z

src/json-patch.ts

+   * @param path The escaped pointer
+   * @return The unescaped path
+   */
+  export function unEscapePath(path: string): string {


The native unescape function does not uppercase the E

felixfbecker · 2017-05-18T12:58:55Z

src/json-patch.ts

+   * @param pointer an escaped JSON pointer
+   * @return The retrieved value
+   */
+  export function getValueByPointer(document, pointer) {


(document: any, pointer: string): any

felixfbecker · 2017-05-18T13:01:06Z

src/json-patch.ts

+   * @param mutateDocument Whether to mutate the original document or clone it before applying
+   * @return `{newDocument, result}` after the operation
+   */
+  export function applyOperation<T>(document: any, operation: Operation, validate: boolean, mutateDocument: boolean): OperationResult;


this overload is unneeded

felixfbecker · 2017-05-18T13:04:20Z

README.md

 * `test` - boolean result of the test
 * `remove`, `replace` and `move` - original object that has been removed
 * `add` (only when adding to an array) - index at which item has been inserted (useful when using `-` alias)

-#### jsonpatch.observe (`obj` Object, `callback` Function (optional)) : `observer` Object
+#### `jsonpatch.applyOperation<T>(document: T, operation: Operation, validate: boolean = false, mutateDocument: boolean = false): T`


signature is outdated

felixfbecker · 2017-05-18T13:17:56Z

src/json-patch.ts

+      }
+    } /* END ROOT OPERATIONS */
+    else {
+      !mutateDocument && (document = deepClone(document));


statement syntax (if) is more readable for side effects than expression syntax imo

felixfbecker · 2017-05-18T13:33:33Z

src/json-patch.ts

+          value = jsonpatch.getValueByPointer(document, (<any>patch[i]).from);
+        }
+        if (patch[i].op == "replace" || patch[i].op == "add") {
+          value = (<any>patch[i]).value;


TypeScript should infer the type as ReplaceOperation | AddOperation if you use === instead of ==, you shouldn't need to cast to any

Didn't work, still complaining. I'm using TS 2.0.0.

Did you try upgrading?

Yes, upgrading breaks compilation if you remember. Fixing the issue needs some investigation and it's not a priority for now.

Yeah, I though it might have been fixed. I would cast to ReplaceOperation | AddOperation then

felixfbecker · 2017-05-18T13:34:51Z

src/json-patch.ts

+
+      }
+      else {
+        results[i] = applyOperation.call(this, document, patch[i], validate, true).result;


why .call(this, ...? What is this even in this context? What does applyOperation need it for?

It needs it for this.validator.

felixfbecker · 2017-05-18T13:35:13Z

src/json-patch.ts

+   * @param operation The operation to apply
+   * @return The updated document
+   */
+  export function applyReducer<T>(document: T, operation: Operation): T;


redundant signature

felixfbecker · 2017-05-18T13:35:25Z

src/json-patch.ts

+   */
+  export function applyReducer<T>(document: T, operation: Operation): T;
+  export function applyReducer<T>(document: T, operation: Operation): T {
+    const operationResult: OperationResult = applyOperation.call(this, document, operation)


why .call(this, ...?

Same reason, it needs this for this.validator.

felixfbecker · 2017-05-18T14:30:25Z

src/json-patch.ts

+  export function applyOperation<T>(document: any, operation: Operation, validate: boolean, mutateDocument: boolean): OperationResult;
+  export function applyOperation<T>(document: any, operation: Operation, validate = false, mutateDocument = true): OperationResult {
+    if (validate) {
+      this.validator(operation, 0);


wait, so if you just call applyValidation(document, op, true) the function will throw? Why does the user have to provide this as a this binding, instead of an optional parameter?

Not really because it is bound to jsonpatch object by default.

As in

var jsonpatch; (function (jsonpatch) { function sub() { return this; } jsonpatch.sub = sub; })(jsonpatch = {}); jsonpatch == jsonpatch.sub(); // ==> true

What if I import it like this

import { applyOperation } from 'fast-json-patch'

and transpile with Babel, or let Webpack do tree-shaking? It probably won't pick up the reference. This feels like a hack

or in ES5:

const { applyOperation } = require('fast-json-patch') const applyOperation = require('fast-json-patch').applyOperation

I'd prefer keeping the API as close as possible as I can to its current shape. Especially when it comes to often-used functions. Setting the validator for once sounds better.

A .setValidator function or a setter sound better, and I'll ask my teammates to maybe throw this whole thing. I don't see any use for it.

Tbh I would consider that a code smell. It's a global setting that could be set anywhere in an application (even by a library) and would yield unexpected results. How would a library customize the validator without affecting application code?

100% legit point.

I would consider that a code smell.

Totally agree.

I'm for the optional param: validate: boolean | Validator, where

false - no validation

true - default validator: jsonpatch.validator

function - custom validator

Perfect then 💃

tomalec · 2017-05-18T13:10:07Z

README.md

@@ -92,29 +92,24 @@ var patch = [
  { op: "add", path: "/lastName", value: "Wester" },
  { op: "add", path: "/contactDetails/phoneNumbers/0", value: { number: "555-123" }  }
 ];
-jsonpatch.apply(document, patch);
+document = jsonpatch.applyPatch(document, patch).document;


the old jsonpatch.applyPatch(document, patch) should work a before, do we need to obfuscate the example code?

tomalec · 2017-05-18T13:12:43Z

README.md


 If the `validate` parameter is set to `true`, the patch is extensively validated before applying.
 An invalid patch results in throwing an error (see `jsonpatch.validate` for more information about the error object).

-Returns an array of results - one item for each item in `patches`. The type of each item depends on type of operation applied
+Returns an array of objects - one item for each item in `patches`, each item is an object `{newDocument, result}`. The type of `result` depends on type of operation applied.


maybe one item for each operation inpatches, each item is an object to reduce the number of "items"

tomalec · 2017-05-18T13:19:38Z

README.md

+TEST_OPERATION_FAILED | When operation is `test` and the test fails, applies to `applyReducer`.
+
+
+#### `jsonpatch.escapePath(path: string): string`


Isn't it a duplicate of the same thing written above?

tomalec · 2017-05-18T16:41:38Z

I'll continue review, later, had to 'finish' it to make github allow me to respond to your comments.

alshakero · 2017-05-21T12:05:46Z

maybe now we could change enigmatic result: Boolean | Object, to removed: Object, test: Boolean.

Can you elaborate on removed: Object, test: Boolean. I don't really understand. A sample return value would do.

What was the rationale with renaming tree to document? Is it used that way in RFC? document is confusing to me as I think of browser's DOM document.

Yes:

JSON Patch defines a JSON document structure for expressing a sequence of operations to apply to a JavaScript Object Notation (JSON) document

Regarding deprecating old apply. What do you think about instead of keeping it backwards compatible but deprecated, break compatibility and make it a regular alias of applyPatch?
Patch in jsonpatch.applyPatch looks quite redundant jsonpatch.apply was quite intuitive. However I may just get used to it.

I don't mind that as a next step, but for now, I guess we should keep warning people to use applyPatch, and that's not to push the name, but to push using the return value. After a month or so we can do whatever we please with apply.

- Add `.newDocument` to applyPatch return value and readme - A little cleanup

felixfbecker · 2017-05-21T12:44:17Z

I like { removed: any, test: boolean }, the return type is less magic that way.

getPointerByValue didn't make use of this and went back to the tree root again.

Is there a reason getPointerValue cannot be changed to do this instead?

alshakero · 2017-05-21T12:56:21Z

@felixfbecker it is changed to a _get now.

tomalec · 2017-05-22T13:48:23Z

src/json-patch-duplex.ts

          for (var i = 0, l = a.length; i < l; i++)
-            if (!_equals(a[i], b[i])) return false;
-
+            if (!_equals(a[i], b[i]))


Could you add {} after if?

tomalec · 2017-05-22T13:50:34Z

src/json-patch-duplex.ts

-      var original = getOriginalDestination.value === undefined ?
-        undefined : JSON.parse(JSON.stringify(getOriginalDestination.value));
+    move: function (obj, key, document) {
+      const originalValue = getValueByPointer(document, this.path);


Can't you use result of const originalValue = applyOperation(document, { op: "remove", path: this.from } ).result; so you don't have to apply _get here and iterate over tree once again?

tomalec · 2017-05-22T13:59:02Z

Speaking of naming of applyPatch and not aliasing it as apply, - then to change apply from deprecated old behavior to just an alias of applyPatch we would have to release major version. I think it would be better not to bump major version too often, especially just to release an alias.

I would consider old behavior buggy and would like to drop support for it sooner than later.
So have jsonpatch.applyPatch <=> jsonpatch.apply (short and easy) @felixfbecker, @warpech WDYT?

Speaking of performance of _get and op: 'move', could you check if it's better now? I also suggested something in review what may help.

Speaking of results:

const removeOutcome = jsonpatch.applyOperation(document, { op: "remove", path: 'path/foo' } );
removeOutcome: {
    document: document,
    removed: 'bar'
};
// replace, move, copy are in fact remove + add
const testOutcome = jsonpatch.applyOperation(document, { op: "test", path: 'patt/foo', value: 'baz' } );
testOutcome: {
    document: document,
    test: true
};

felixfbecker · 2017-05-22T14:35:51Z

I would say either

deprecate apply (but keep old behaviour), release a minor
or remove apply, release a major

alshakero · 2017-05-22T15:49:21Z

I would consider old behavior buggy and would like to drop support for it sooner than later.
So have jsonpatch.applyPatch <=> jsonpatch.apply (short and easy)

I'd keep buggy apply and release minor, not because I don't agree with you at all. But because I think we have a necessary update that must be major (changes files hierarchy). So I think we should save major release for that, the update is to go for proper modularization:

Using CommonJS in files, so duplex will use json-patch.ts, and by this:
we can use (read require) thoroughly tested libraries for _equal and deepClone instead of making it our problem like now.
This will mean less code, fewer bugs, and all applyX functions will be in one file. It was hell copying and pasting with every modification I make.
This will remove the hacky exporting we're doing right now that breaks browersify apps.
It will make moving to proxies easier for next-next version 3.0.0.

As big as this may sound. It's exactly two hours for me, I don't even mind doing it in this PR today. With benchmarking clone lib vs our deepClone. If you agree, I can go for it and we can remove apply just now.

Speaking of performance of _get and op: 'move', could you check if it's better now? I also suggested something in review what may help.

I did, it's back as it was 💃

Regarding results, I'm working on it.

felixfbecker · 2017-05-22T15:54:00Z

I would vote for keeping a minor and not bloating this PR any further.

alshakero · 2017-05-22T15:55:16Z

@felixfbecker Me too. I'm just trying my best to point out that it's not going to take a lot of time to implement it. And that we need a major release for something else.

- Update tests to follow. - Explain `OperationResult` clearly in README. - Return overwritten value in `move` operation.

tomalec · 2017-05-22T20:30:25Z

Thanks, now I feel convinced :)

tomalec

Maybe we should also return result.removed for add operation, as it may replace:
https://tools.ietf.org/html/rfc6902#section-4.1

If the target location specifies an object member that does exist,
that member's value is replaced.

tomalec · 2017-05-22T20:32:27Z

src/json-patch-duplex.js

-            return originalValue;
+            var overwrittenValue = getValueByPointer(document, this.path);
+            if (overwrittenValue) {
+                overwrittenValue = deepClone(overwrittenValue);


Why do we need to clone for move?

This is how it originally behaved. In case the move target was overwritten we need to keep a copy of it to return it as removed. I can remove this but there is a spec for it and I thought it's necessary.

Ok, fair point.
However, I'm not 100% sure if it's very useful, but it could be expensive for large object.

Could you add short code comment, jsdoc, or readme node why it's needed? We could discuss it later if we would like to improve performance.

Sure! A couple minutes.

tomalec · 2017-05-22T20:33:30Z

src/json-patch-duplex.js

+                            }
+                        }
+                        else {
+                            debugger;


Please remove this debugger

tomalec · 2017-05-22T20:36:23Z

src/json-patch-duplex.js

+                        }
+                        else {
+                            debugger;
+                            returnValue.removed = arrOps[operation.op].call(operation, obj, key, document); // Apply patch


I think it would be cleaner if each arrOps[*] would return applicable object {remove: smth} or {test: true} so you will not have to check it once again here.

tomalec · 2017-05-22T20:36:38Z

src/json-patch-duplex.js

+                            }
+                        }
+                        else {
+                            debugger;


please remove debugger

tomalec · 2017-05-22T20:37:25Z

src/json-patch-duplex.js

+                        }
+                        else {
+                            debugger;
+                            returnValue.removed = objOps[operation.op].call(operation, obj, key, document); // Apply patch


Same as above. Maybe we don't need this if at all?

tomalec · 2017-05-22T20:37:34Z

src/json-patch-duplex.js

@@ -593,7 +606,9 @@ var jsonpatch;
                Object.keys(value_1).forEach(function (key) { return document[key] = value_1[key]; });
            }
            else {
-                results[i] = applyOperation(document, patch[i], validateOperation, true).result;
+                results[i] = applyOperation(document, patch[i], validateOperation);
+                debugger;


debugger once again

tomalec · 2017-05-22T20:37:57Z

src/json-patch-duplex.js

-                results[i] = applyOperation(document, patch[i], validateOperation, true).result;
+                results[i] = applyOperation(document, patch[i], validateOperation);
+                debugger;
+                results[i] = results[i].removed || results[i].test;


Can the results array be an array of result objects?

Not really, because this is deprecated apply.

tomalec · 2017-05-23T09:29:39Z

src/json-patch-duplex.js

-                        else {
-                            debugger;
-                            returnValue.removed = arrOps[operation.op].call(operation, obj, key, document); // Apply patch
+                        returnValue = arrOps[operation.op].call(operation, obj, key, document); // Apply patch


Now, we don't need to create returnValue above, to replace it here .

alshakero · 2017-05-25T16:04:24Z

@tomalec shall I merge?

felixfbecker · 2017-05-28T18:31:59Z

src/json-patch-duplex.ts

+      results[i] = applyOperation(document, patch[i], validateOperation);
+      document = results[i].newDocument; // in case root was replaced
+    }
+    (<any>results).newDocument = document;


This should be reflected in the return type, e.g.

export interface PatchResult<T> extends Array<OperationResult<T>> { newDocument: T; }

And initialize results array like this?

const results = (<PatchResult<T>>new Array(patch.length));

yeah, the cast is needed since at the time of initialising newDocument is not set even though it's not optional. You can also write the slightly prettier

const results = new Array(patch.length) as PatchResult<T>;

Btw, is there a perf benefit in using the array constructor? I once read that array literals are faster

According to this, the constructor is ~200% faster.
https://jsperf.com/array-constructor-vs-literal-modalities

Implement applyPatch to fix #147

a2709e7

alshakero requested review from tomalec and warpech May 10, 2017 09:21

alshakero mentioned this pull request May 10, 2017

Replacing root object with an array doesn't work #147

Closed

felixfbecker suggested changes May 10, 2017

View reviewed changes

Make applyPatch reduce-friendly

e122a94

- Enhance typings - Verify if validate is a boolean or default to false - Throw an error when `test` operation fails - Change tests accordingly - Add documentation for using applyPatch with reduce

tomalec reviewed May 10, 2017

View reviewed changes

felixfbecker reviewed May 10, 2017

View reviewed changes

Enhance applyPatch function

947f80d

- Turn to opt-in pure function - Better documentation - Needed tests

felixfbecker suggested changes May 11, 2017

View reviewed changes

felixfbecker and others added 6 commits May 15, 2017 10:53

Rename Patch to Operation

f285fd7

Fix bug

7e32df3

Improve docs

e4fe8ac

Merge pull request #172 from felixfbecker/applyPatch-for-root-operations

20c0e91

Rename Patch to Operation

Prettier code

7c10989

felixfbecker suggested changes May 18, 2017

View reviewed changes

felixfbecker reviewed May 18, 2017

View reviewed changes

tomalec requested changes May 18, 2017

View reviewed changes

alshakero added 2 commits May 21, 2017 14:08

Use for instead of for...of

e502d45

- Add `.newDocument` to applyPatch return value and readme - A little cleanup

Apply modifications to json-apply.ts (forgot)

bfa1d22

tomalec requested changes May 22, 2017

View reviewed changes

Clearer OperationResult value

2f092a2

- Update tests to follow. - Explain `OperationResult` clearly in README. - Return overwritten value in `move` operation.

tomalec requested changes May 22, 2017

View reviewed changes

Make ops return and remove debugger

0d1d2a9

tomalec requested changes May 23, 2017

View reviewed changes

Move returnValue in scope, explain deep cloning in move

a1faf3f

felixfbecker suggested changes May 28, 2017

View reviewed changes

tomalec approved these changes May 29, 2017

View reviewed changes

Add PatchResult type

3199b2a

felixfbecker approved these changes May 29, 2017

View reviewed changes

felixfbecker mentioned this pull request May 29, 2017

Fix issue #76 by using deep clone #134

Merged

alshakero merged commit d60fc5c into master May 29, 2017

This was referenced May 29, 2017

Make applyOperation a standalone function and use it inside applyPatch #175

Closed

Export function to escape pointers #173

Closed

This was referenced Jun 8, 2017

Replace parseInt with bitwise operation #171

Closed

Undocumented behavior: jsonpatch.compare alters obj1 #93

Open

alshakero mentioned this pull request Jan 20, 2018

Support Comparing Map #198

Closed

alshakero mentioned this pull request Jun 2, 2018

Consider changing root operations behavior #113

Closed


		Applying patches on root is a little tricky, for that you can't replace your root object with an object of a different type in place (eg. can't replace `{item: "item 1"}` with `[{item: "item 1"}]`) on the root level, because the original document is an object `{}` and the new value is an array `[]`, nor can you do the opposite (`[] => {}`), obviously. See https://github.com/Starcounter-Jack/JSON-Patch/issues/147.

		However, you can use `applyPatch` method to over come this issue, because it returns the result object, and this allows it to change types.

		Applies a single `patch` on `obj`.

		Returns the modified object.


		Available in json-patch.js and json-patch-duplex.js

		Applies a single `patch` on `obj`.

		TEST_OPERATION_FAILED \| When operation is `test` and the test fails, applies to `applyReducer`.


		#### `jsonpatch.escapePath(path: string): string`

Implement applyPatch to fix #147, with all needed specs #167

Implement applyPatch to fix #147, with all needed specs #167

Conversation

alshakero commented May 10, 2017 • edited Loading

felixfbecker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero commented May 11, 2017

felixfbecker commented May 11, 2017

felixfbecker commented May 11, 2017

alshakero commented May 11, 2017

felixfbecker May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented May 12, 2017

alshakero commented May 15, 2017

felixfbecker commented May 15, 2017

alshakero commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero commented May 10, 2017 •

edited

Loading

felixfbecker left a comment •

edited

Loading

alshakero May 11, 2017 •

edited

Loading

felixfbecker May 11, 2017 •

edited

Loading

alshakero May 11, 2017 •

edited

Loading

alshakero May 18, 2017 •

edited

Loading