[PR-20-delete-support] Add support for delete.

[chrishalbert]

Add support for delete() operation.

[flitbit]

I might have been fixated on a new project, just saw this so I'll review and get it merged. First glance looks pretty good. Thanks for the tests.

[chrishalbert]

Great, thank you! I was assigning null in the meantime for my use case, but that ended up creating another type of bug in my app.

[flitbit]

I'm curious about the use case. The implementation and semantic seems symmetrical with the .set() method, but not in line with the library's purpose.

Could: .set(path, undefined) but .delete(path) feels better.

[chrishalbert]

@flitbit I'm not sure what you mean by the use case, but it was basically to have the inverse operation - if I can set a value to one object, I would like to also delete a value too. I was trying to move it from obj a => obj b. The move seemed well outside the scope of the library so I was trying to propose the underlying functionality, which couple w/ set, could achieve my use.

The bug that came up in my app was specific to how the middleware was handling nulls vs. undefine, but you are totally right about the suggestion.

Yes, I used your original code adding the delete operation due to its performance, but you are totally correct, .set(path, undefined) would get the job done. I hadn't thought of that. I could change delete() to simply call set() as you specify, update the README and resubmit if you would like? That way it feels nice, but fulfills DRY and keeps the repo tiny. Let me know your thoughts! Thanks again.

[flitbit]

I think a specific operation is better because the .set() method creates the path to the element while the delete can just stop anytime it encounters an undefined.

How do you feel about .unset() instead of .delete() since it is the inverse of .set().

[flitbit]

@chrishalbert Would you be so kind as to test drive the updated version, which includes your changes, although I had to merge into my ts branch which I've been tooling on for a while; 100% test coverage, typescript, retooled build etc.

There is a pre-release package: npm install json-ptr@1.2.1-ts.93dc79c.0

@treybrisbane you may want to check this out too... I know you've been waiting for the typings to get published. Might have gone to far getting 100% coverage.

From my perspective I'm ready to publish v1.3 (except I'm updating the readme first).

[chrishalbert]

@flitbit - Will do! I'll install today, run my test suites which rely on it (a meager 98% compared to your 100% :P ) and then let you know how it goes. Thanks again!

[flitbit]

Previously I think it was below 50% of branches covered but 90+% of lines. Got 100, 100, 100 :o).

[chrishalbert]

@flitbit Sorry for the late reply. I saw that you released 1.3 so I upgraded from 1.2.* => 1.3.1 - I tried rerunning my tests and had several failing - I haven't had a chance to debug yet, but I should be able to take a look by end of day (node 12.16.1) - I'll keep you posted

[chrishalbert]

@flitbit Alls well! This was the change that affected me: import { JsonPointer, create } from 'json-ptr'; so it was an easy fix. On that note, I didn't see anything about semver, but would this warrant a 2.0 release for backwards incompatible changes but more so really for the typescript overhaul? Only reason I point out is because last week I saw the same type of library change affect another - there wasn't enough app code coverage to catch it.

[flitbit]

@chrishalbert The semver mistake is mine; we whipped that dead horse in this issue... my apologies.