Closed cloydlau closed 4 months ago
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
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
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.
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!
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?
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.
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.
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. 🤔
💡 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).
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
/>
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.
I've fixed the issue with set(...)
not triggering an onChange
event in v0.18.2
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.
@MitchBuell I think you are looking for this idea (not yet implemented): #145
I've been thinking a bit more about a solution to support mutable changes.
I see two main challenges:
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.
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 https://github.com/josdejong/svelte-jsoneditor/issues/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?
I think we're discussing two interesting points here, maybe best to keep them separate:
onChange
for programmatic changes or not (see #128, #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.
OK, let's keep them separate:
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.
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 |
× | × | × | × | × |
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.
Thanks for the update.
My action points for now are:
onChange
to only trigger on user actions again, not programmatic actions anymore.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.
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}
).
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.
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.
onChange
triggers, I want to address that in a separate PR. #332 is only about (im)mutability.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);
}
"
>
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.
I'm not sure whether we're on the same page.
There are two things that I want to address:
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?
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:
Thanks for thinking along Cloyd.
Some thoughts:
.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.
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.
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 withupdate
, the view won't update.