fix: when resetting `mode` to the default value (`undefined`), the mode switches successfully, but the corresponding menu item is not selected

cloydlau commented 2 months ago

Hello Jos, I've found a small issue and I'm sending a PR for your review.

Minimal reproduction for the small issue:

<!doctype html>
<html lang="en">
  <body>
    <div id="jsoneditor"></div>
    <button onclick="resetMode()">reset mode</button>
    <script type="module">
      import { JSONEditor } from 'https://cdn.jsdelivr.net/npm/vanilla-jsoneditor/standalone.js'

      const editor = new JSONEditor({
        target: document.getElementById('jsoneditor'),
        props: {
          content: {
            text: '',
          },
          mode: 'text',
        },
      })

      window.resetMode = () => {
        editor.updateProps({
          mode: undefined,
        })
      }
    </script>
  </body>
</html>

josdejong commented 2 months ago

Thanks Cloyd!

I think that other places in the code can suffer from similar issues: the TypeScript definition of mode in JSONEditorRoot is Mode, and not Mode | undefined, but via the updateProps method it is possible to set this option to undefined. The same holds for other options. To solve this for real, I think we can go two directions:

In JSONEditor.svelte, change updateProps so that it makes sure that all required properties cannot be set to undefined, and instead, set them to their default (which is Mode.tree in the case of mode).
In JSONEditor.svelte, do not give the options properties defaults, and instead have them all defined with undefined as possible value. Then, where the options are used, define them like mode={mode || defaultMode} so they will always be defined in the rest of the application. Or: define intermediate variables like $: modeOrDefault = mode || defaultMode and use that everywhere to prevent repeating mode || defaultMode if needed.

Option 2 sounds best to me. What do you think?

cloydlau commented 2 months ago

Is option 2 a bit burdensome? Does it require maintaining an additional defaultXXX for each property?

export interface JSONEditorPropsOptional {
  mode?: Mode
  validator?: Validator | null
}

I think the ? (optional property of TS) here already covers the case of undefined, it's just that now it seems that not passing mode at all and passing mode but set to undefined are behaving differently.

And there is a TS compiler option for this situation: https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes I'm not sure if enabling this option is a good idea though.

josdejong commented 2 months ago

The code indeed uses optional properties and defaults, however you uncovered that these defaults do not always work, so the properties can become undefined despite having a default. So, everywhere the properties like mode are used, we need to make sure to handle the case that it may be undefined. I would like to minimize the number of places where we have to recon with that. It's probably easiest to see how it works out by trying it out in a PR, then we can see if we need something like modeOrDefault.

cloydlau commented 2 months ago

I submitted some code, though I feel there are definitely omissions, could you make any additions based on this if my understanding is correct?

josdejong commented 2 months ago

Thanks for getting this started!

This is indeed the gist of it, though

export let defaultMode = Mode.tree
export let mode: Mode | undefined = defaultMode

can become:

let defaultMode = Mode.tree       // No need to export. Also: maybe easier to name it modeDefault?
export let mode: Mode | undefined // no need to fill in a default value

And then, we need to apply the same trick for all component properties.

josdejong commented 2 months ago

O: and, we should do this in JSONEditor.svelte rather than JSONEditorRoot.svelte

cloydlau commented 2 months ago

The default values of properties could be used in multiple places, so my latest commit have moved them to a common file. What do you think?

josdejong commented 2 months ago

Hm, that may be handy, I'm not sure. The defaults are only needed inside JSONEditor.svelte, so maybe it only introduces overhead.

If you want I can work out a solution, it's on my list. I'm not sure if I can make it today though.

josdejong commented 2 months ago

Some of the properties are currently defined like selection: Selection | null. Defining those now as selection: Selection | null | undefined is ugly, so maybe good to change it into selection: Selection | undefined (a breaking change).

josdejong commented 2 months ago

O, we do have to define the properties like export let mode: Mode | undefined = defaultMode otherwise the property will not be seen as optional by Svelte.

josdejong commented 2 months ago

Here a start: https://github.com/josdejong/svelte-jsoneditor/pull/426/

Still need to work out the details.

SamMousa commented 1 month ago

Hmm, my thoughts on this:

updateProps internally uses $set: https://svelte.dev/docs/client-side-component-api#$set This is an issue because this exposes all internal variables of the components, not just the exported ones. A solution would be to only take the relevant properties. In TS land this will be fine since we have type checks, but in JS land there are no types so any property with any name or type will be written to the component property.
Since there's only 1 public API to change these properties it is feasible to do needed checks inside that function. For this case specifically we could iterate over the object and remove all undefined values.
We can compile the component with accessors: true, this will expose the props (so only the variables defined with export). This would allow the user to update any prop individually.
I'd consider taking a full object of new props might not make sense and it'd be better to just recreate the component.
All of the above does not actually solve the issue of resetting something to the default value.

If we really need updateProps() we could implement it somewhat like this:

export async function updateProps(props: Record<string, unknown>): Promise<void> {
    for (let prop in props) {
        if (props[prop] !== undefined) {
            // Lazy (me as a coder) example
            this.$set({prop: props[prop]);
            // Not sure if this works, might require compiling with accessors. Might be synchronous which is not ideal if updating many properties
            this[prop] = props[prop];
        }
    }
    await tick() // await rerender
  }

In conclusion, I'd consider fully removing updateProps and just compiling with accessors!

josdejong commented 1 month ago

Thanks for your inputs Sam, you have some very good points!

First: to me, the updateProps method is very valuable to me because it makes it a no-brainer to integrate svelte-jsoneditor with a framework like React: when you create a React wrapper around the library, you can easily pass all properties from React to the library with one line of code. If we would only expose individual accessors for every property, you would have read every individual property from the props object and then call the right setter method. And you have to update this wrapper code when a new property is introduced in a future version of the library.

I'm not really happy with how #426 is shaping up, because it moves the burden of dealing with undefined cases everywhere the properties are used. So maybe my option 2 is not the best approach, and we should look into improving updateProps itself in a self-contained way like you propose.

For context: I was reading up on https://github.com/sveltejs/svelte/issues/9948 and https://github.com/sveltejs/svelte/issues/9764, about whether a property falls back to its default value when changing it to undefined.

When looking into option 1, improving updateProps, I think we can go in two directions:

1.1 Ignore/disallow undefined values (Sam's proposal) or throw an error. I think it would be more neat to throw an error instead of silently ignoring props.
1.2 Change updateProps such that in case of an undefined value, it actually applies the default value for that property.

I'm not sure how broad of an issue this is, and it seems that Svelte 5 gets better support for this so after migrating it may be that this issue is solved as a side effect. So maybe for now the approach of 1.1 is best, and when migrating to Svelte 5 (once released) we can look into ensuring the fallback to default values to actually work.

What do you guys think @cloydlau @SamMousa ?

cloydlau commented 1 month ago

I recommend not making any changes before upgrading to Svelte 5, reassess after the upgrade.

josdejong commented 1 month ago

Yeah, let's await Svelte 5. (we can already experiment with it if anyone is into it)

SamMousa commented 1 month ago

I only reacted to this because of code quality concerns; I don't care about this feature otherwise so I'll not experiment with it.

That said, an improvement unrelated to the default values is to rewrite updateProps() to not allow people overriding private internal variables.

A simple solution for the default values could also be implemented with the following approach:

Import the svelte component inside its module script.
Instantiate a singleton component without setting any props (this component therefore has all props set to their default).
When using updateProps() make it take the default value from the singleton if undefined is provided.

This approach will:

Keep your code clean, defaults (or as svelte calls them: initial values) are defined as expected
Allow you to remove the extra boilerplate when Svelte 5 lands and improves the situation

Note that importing a svelte file from itself might technically not be intended; I've used it in some other projects and it works though...

josdejong commented 1 month ago

Thanks for your inputs Sam! We can indeed improve on updateProps in multiple ways.

I don't see "overriding private internal variables" as a major issue, though it is good to keep it in mind. In most JavaScript code you can easily override methods and public/private variables and even override built-in globals like window. But the only thing you achieve with that is messing up your own app. The TypeScript types help a lot by discovering features and also wrong usage of the API though.

josdejong / svelte-jsoneditor

fix: when resetting `mode` to the default value (`undefined`), the mode switches successfully, but the corresponding menu item is not selected #424