Programmatic changes do not trigger 'onChange'

[cloydlau]

Hello Jos,
The doc says 'The onChange callback which is invoked on every change of the contents, both changes made by a user and programmatic changes made via methods like .set(), .update(), or .patch()'.
But now it isn't, so is it intended or a bug?
Reproduction Link: https://codesandbox.io/s/svelte-jsoneditor-vue-forked-gp453r?file=/src/components/VueJSONEditor.vue
And another problem is that if I replace the set method in the Reproduction Link with update, the view won't update.

[josdejong]

Thanks for reporting. The onChange in your example indeed doesn't work. Can you simplify the demo further?

I started from a bare bone JS demo testing updateProps, update, and set, and in that simple case onChange works fine with updateProps and update. I'll look into that next week.

https://jsbin.com/jimehuh/edit?html,output

UPDATE: I had an error in my demo, method set works but doesn't trigger an onChange event

[josdejong]

Can you strip the demo of the issue further so we can triangulate the issue?

Can you also update to use the latest version of the library? You're demo uses v0.5.0, the latest version is v0.16.1

[josdejong]

The issue may be related to your example making a nested change in the existing content (mutating it). If you don't mutate but instead create and pass an updated object, the editor apparently doesn't always detect that the contents changed when you pass in the same object.

[cloydlau]

Hey, I've created a new vanilla js & latest version of vanilla-jsoneditor & minimal reproduction, see: https://github.com/cloydlau/reproduction.git
I listed 4 cases, check it out!

[josdejong]

I've experimented with implementing support for mutating JSON objects. The editor is built around the assumption that the data is immutable. This has a number of big memory and performance advantages. Without immutability, the editor will have to make a full copy of the document with every change that is made, and it will have to re-render the full document as well. With immutable objects, it only has to render the changed parts and does not have to create deep copies.

What I can do is create an option { immmutable: boolean }, and when false, let the methods get(), set(), update(), and callback onChange create a deep copy of the document. This will work fine with small documents. I think it would be best to set immutable: false by default (that is the safest approach). Does that make sense?

[josdejong]

I've created a PR testing this out with a new option { immutable: boolean } on the editor. The performance impact is really bad (deep copying the document on every change), and only needed when people do programmatic changes themselves outside of the editor. So I'm in doubt whether it is a good idea to set { immutable: false } by default, giving everyone the slow-but-safe editor by default.

An other option would be to just document the current behavior and explain that you should not mutate your data.

[cloydlau]

		set with mutated json	set with new json	update with mutated json	update with new json	patch
v0.18.11	UI re-rendered	√	√	×	√	√
v0.18.11	onChange triggered	×	×	×	√	√
immutable === false	UI re-rendered	√	√	√	√	√
immutable === false	onChange triggered	×	×	√	√	√
immutable === true	UI re-rendered	√	√	×	√	√
immutable === true	onChange triggered	×	×	×	√	√

According to the test results, the issue seems not fully solved by that PR.
And I think whether or not to trigger the onChange event after programmatic changes should be reconsidered.
For native behavior, onchange event will not be triggered when you make programmatic changes:

<input type="text" onchange="onInputChange()">
<button onclick="setInputValue()">set input value</button>
<script>
  function onInputChange () {
    console.log('onInputChange')
  }
  function setInputValue () {
    document.querySelector('input').value = Math.random()
  }
</script>

Could you elaborate further on this: 'The editor is built around the assumption that the data is immutable'?
I guess two-way binding is built on top of mutation, the content is actually changed even if you don't call set or update.
I'm not familiar with Svelte, is there a better way to trigger re-rendering after mutating? In Vue you can deep watch a object instead of creating a deep copy.

[josdejong]

Thanks for your thorough testing.

The idea is that onChange triggers after both user interactions and programmatic interactions as described in the docs. I just did a quick test and onChange works like that when using the library within Svelte, but not with the .set method when using it outside of Svelte. I'll look into that, it's not a regression in this PR but an existing bug. I opened #333 for that.

I'm not familiar with Svelte, is there a better way to trigger re-rendering after mutating? In Vue you can deep watch a object instead of creating a deep copy.

In Svelte, mutating an object like users[0].name = 'foo' works too, like in Vue. It will rerender everything users, it is not fine grained (does Vue rerender that in a fine-grained way? Or also rerender everything?). So when staying withing either Vue or Svelte, we would be sort of fine I guess.
The difficulty comes in when passing a plain object to a plain JavaScript function, like .set() or .update() or any function basically. If you change one of the nested properties of that object, and pass the object again to a JS function, there is no way to tell the difference, unless you make a copy of the previous version so you can do a deep comparison. But that is very expensive. 🤔

[josdejong]

💡 One thing I can try out is turning off <svelte:options immutable={true} /> that I use on all all components. It will probably render mutated changes as we want. Will have to see what that does with the performance (and think though how to make that customizable if it is indeed a solution).

[cloydlau]

Actually I tested within Svelte environment (with your codebase).
Vue rerender that in a fine-grained way. But if Svelte will rerender too when nested properties are mutated, what confuses me is why this doesn't work:

<script>
import { JSONEditor } from '../../../src/lib'

let jsonEditorRef
let content = { json: { a: 1 } }

function onClick() {
  content.json.a = Math.random()
  console.log(jsonEditorRef.get()) // will get new value, but UI doesn't rerender
}
</script>

<button on:click={onClick}>directly mutate content</button>
<JSONEditor
  bind:this={jsonEditorRef}
  bind:content
/>

[josdejong]

The code base of svelte-jsonedior currently has <svelte:options immutable={true} /> in every component, I think that is the reason why a change in nested props does not propagate. It's on my list to test that out.

[josdejong]

I've fixed the issue with set(...) not triggering an onChange event in v0.18.2

[MitchBuell]

Not to hijack this Issue, but I actually would like a way to programmatically modify the JSONEditor contents without triggering an onChange. I'm using the vanilla version in a React app, and I've tried unregistering/update/re-registering but the useEffect is so fast that doesn't work, the onChange is always called.

[josdejong]

@MitchBuell I think you are looking for this idea (not yet implemented): #145

[josdejong]

I've been thinking a bit more about a solution to support mutable changes.

I see two main challenges:

1. History. For undo/redo we must be able to restore a previous version of the document. That can be done either by storing a copy of the previous version, or storing the reverse patch operations needed to re-create the previous version based on the current version of the document. For internal actions we can use json patch. But for external changes we can’t. Therefore, we need to keep a copy of the previous version of the document. In order to support mutable changes, we'll need to make a copy of the document on every change.
2. onChange. To properly emit onChange events, we need to be able to know when the document changes. When the document is changed by mutating it, it would be complex to keep track of actual changes in Svelte due to the two-way binding: an change can be triggered by an actual change, or "just" an internal re-render, and you can't know. Therefore, internally we best keep the API immutable and have an explicit content and previous content.

In conclusion: if we want to support mutations, we do need to make a copy of the document on every change to support undo/redo and onChange events. Internally, the library can keep using an immutable API. The immutable API is performant and suitable for large documents. The mutable API can be enabled with a configuration option and is suitable for small documents.

So, I think my earlier experiment #332 is in the right direction and we can work that out in detail. I think it will enable the best of both worlds.

[cloydlau]

I personally believe that undo/redo is primarily intended for user interactions rather than programmatic changes. Undo/redo for programmatic changes can also be handled programmatically.

The idea is that onChange triggers after both user interactions and programmatic interactions as described in the docs.

The doc says: 'both changes made by a user and programmatic changes made via methods like .set(), .update(), or .patch()', but it does not specify the reason for that.

Other than the use case mentioned in #128, it seems that triggering the 'onChange' event when making programmatic changes is not a mainstream practice (such as the native input element).

Is it a possible solution to disregard undo/redo and onChange event when making programmatic changes?

[josdejong]

I think we're discussing two interesting points here, maybe best to keep them separate:

1. supporting mutable changes (this topic)
2. triggering onChange for programmatic changes or not (see onChange not called when .patch() applies JSON patches #128, Flag for programmatic vs UI changes #145)

The reason for triggering onChange for programmatic change too is indeed discussed in #128. The editor is now robust against circular events, making this possible. Still, I'm not sure if it was a good move. This kind of solution is prone to infinite loops. And you have a good point that this is not common behavior (the native input behavior is a good one). One solution would be adding a flag to distinguish between user and programmic changes, discussed in #145. Another one would be to revert, and only fire onChange on user changes again. When you need it triggered on programmatic changes too, you can always trigger it yourself. @RyKilleen any more thoughts in this regard?

Is it a possible solution to disregard undo/redo and onChange event when making programmatic changes?

I think that would be problematic and lead to an odd and furstrating user experience: sometimes you can undo, sometimes you can't. But I don't think it would solve the issue of supporting mutable changes.

[cloydlau]

OK, let's keep them separate:

1. mutable changes

UI re-rendered	set with mutated json	set with new json	update with mutated json	update with new json	patch
v0.18.11	✓	✓	×	✓	✓
immutable === true	✓	✓	×	✓	✓
immutable === false	✓	✓	✓	✓	✓

I think the current behavior is the expected behavior, mutable changes support is not necessary.

2. triggering onChange for programmatic changes or not

onChange triggered	set with mutated json	set with new json	update with mutated json	update with new json	patch
v0.18.11	×	×	×	✓	✓
immutable === true	×	×	×	✓	✓
immutable === false	×	×	✓	✓	✓

The current behavior is problematic whether we trigger or not.

I think there are 3 solutions for this:

onChange triggered	set with mutated json	set with new json	update with mutated json	update with new json	patch
Solution 1: compliant with the document, need mutable support	✓	✓	✓	✓	✓
Solution 2: compromise proposal, keep immutable	×	✓	×	✓	✓
Solution 3: native <input> behavior, keep immutable	×	×	×	×	×

3. undo/redo

sometimes you can undo, sometimes you can't

I think user can always undo/redo when there're only user operations. Programmatical changes are system behaviors and should be treated separately from user operations. I mean, user can undo/redo what they have done, they should not undo/redo what they have not done.

[josdejong]

Thanks for the update.

My action points for now are:

1. I'm strongly considering to change onChange to only trigger on user actions again, not programmatic actions anymore.
2. I'll work out feat: create option immutable, implement support for mutable changes #332, introducing a property immutable so we can support both mutable changes, and high performant immutable changes.

About the undo/redo: the user must be able to undo programmatic changes, else the whole thing doesn't work. Suppose the user has a list with 20 names, and edits the 16th. Then, programmatically, the last 10 names are removed from the list, leaving only the first 10 names. If the user wants to undo his only change (renaming the 16th name), he will first have to undo removing the last 10 names from the list, because the 16th name doesn't exist after the programmatic change.

[josdejong]

#332 is now working.

I'm still a bit unsure about what will be the best default value for the new option immutable. And I'm not entirely happy with the added complexity of having to support both mutable and immutable ways of working.

In the PR right now, when creating the editor, it will by default use {immutable: false} and throw a warning to either configure the editor with {immutable: true}, or configure it with {immutableWarningDisabled: true} to silence the warning and keep using the mutable support. Since the mutable support is really bad for performance, I would like people to make a conscious choice instead of silently giving them either the bad performance ({immutable:true}) or an editor that doesn't pick up mutable changes ({immutable:false}).

[cloydlau]

I personally believe the mutable change support is unnecessary.

onChange triggered	set with mutated json	set with new json	update with mutated json	update with new json	patch
Solution 1: compliant with the document, need mutable support	✓	✓	✓	✓	✓
Solution 2: compromise proposal, keep immutable	×	✓	×	✓	✓
Solution 3: native <input> behavior, keep immutable	×	×	×	×	×

I suggest adopting either solution 2 or 3. And document the behavior.

[josdejong]

I personally believe the mutable change support is unnecessary.

As far as I can see, it is necessary to support history (undo/redo), and help Svelte optimize by rendering only changed parts of the UI. And an editor without undo/redo is not acceptable to me, that is really necessary for a good user experience. If you know of an alternative solution please let me know, the deep copying is far from ideal.

#332 does not address changing when onChange triggers, I want to address that in a separate PR. #332 is only about (im)mutability.

[josdejong]

I personally believe the mutable change support is unnecessary.

I'm a bit confused: the mutable support is necessary to get the following part from your example working:

    <button
      @click="
        () => {
          content.json.greeting = Math.random(); // <-- mutable change
          editor.set(content);
        }
      "
    >

[cloydlau]

Yes that's the current behavior of 'set with mutated json', but not necessarily the expected behavior.

onChange triggered	set with mutated json	set with new json	update with mutated json	update with new json	patch
Current behavior or #332 with immutable === true	×	×	×	✓	✓
#332 with immutable === false	×	×	✓	✓	✓
Solution 1: compliant with the document, need mutable support	✓	✓	✓	✓	✓
Solution 2: compromise proposal, keep immutable	×	✓	×	✓	✓
Solution 3: native <input> behavior, keep immutable	×	×	×	×	×

Solution 1 indeed need mutable support, but solution 2 or 3 seem not.

[josdejong]

I'm not sure whether we're on the same page.

There are two things that I want to address:

1. Implement support for mutable changes, solving that the UI does not re-render after a mutable change. See #332
2. Change the onChange callback to not trigger on programmatic changes. I've just created #410 to address this.

So #410 will address the "onChange triggered" issue, and #332 will address the "UI re-rendered" issue. Does that make sense?

[cloydlau]

UI re-rendered	set with mutated json	set with new json	update with mutated json	update with new json	patch
Current behavior or #332 with immutable === true	✓	✓	×	✓	✓
#332 with immutable === false	✓	✓	✓	✓	✓

The editor is built around the assumption that the data is immutable.

I think this means the mutations to the data won't trigger UI re-render directly, but it's maybe a different story when we explicitly call the set/update/patch methods.

I think there are two solutions:

1. If the cost of solving the "UI re-rendered" issue is the necessity to implement mutability, perhaps we can leave it as it is and document it.
2. We should figure out is it possible to trigger UI re-render when set/update/patch methods are called no matter with a new data or a mutated old data without implementing mutability.

[josdejong]

Thanks for thinking along Cloyd.

Some thoughts:

1. I will go with your solution 1 for now: merg #410 and document that changes must be immutable. If it turns out to be problematic for many users we can always merge #332 to add mutable support. I have the feeling though that it isn't that broad of a problem. And since the issue is detectable during development, it will cause WTF's but the docs will explain.
2. The issue is not only about UI re-rendering but also about supporting history (undo/redo). The UI-rendering may be solvable, but requires not optimizing Svelte components for immutability, which worsens performance. The support for history with mutable changes is not solvable except by deep copying the document (as done in #332).
3. Mutable changes are currently not detected by .update, like you show in your table. But it is also problematic with .patch and with a two-way binded variable in Svelte. A re-render is triggered in these cases but that is only thanks to side effects like the selection being changed. But with mutable changes, the history (undo/redo) doesn't work reliable.

Ok, I'll merge #410 now and update the docs explaining about immutability.

[josdejong]

I've just published v0.22.0, where the onChange callback does not trigger anymore for programmatic changes, and where the docs explain about the need to make immutable changes only.