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

cloydlau · 2024-04-14T10:25:52Z

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>

…nu item is not selected

…o fix

josdejong · 2024-04-17T15:01:52Z

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 · 2024-04-21T06:33:51Z

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 · 2024-04-21T06:39:36Z

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 · 2024-04-21T07:09:06Z

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 · 2024-04-24T12:24:11Z

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 · 2024-04-24T12:38:01Z

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

cloydlau · 2024-04-24T12:40:07Z

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 · 2024-04-24T12:49:20Z

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 · 2024-04-24T12:56:25Z

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 · 2024-04-24T13:10:47Z

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.

…ult value for the property

josdejong · 2024-04-24T13:31:44Z

Here a start: #426

Still need to work out the details.

SamMousa · 2024-05-16T14:10:38Z

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 · 2024-05-17T08:01:11Z

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 sveltejs/svelte#9948 and sveltejs/svelte#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 · 2024-05-21T00:53:56Z

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

josdejong · 2024-05-21T06:59:51Z

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

SamMousa · 2024-05-21T07:39:34Z

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 · 2024-05-21T17:02:51Z

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.

cloydlau added 2 commits April 14, 2024 18:21

fix: when resetting mode to the default value, the corresponding me…

68fef17

…nu item is not selected

fix: [warn] Code style issues found in the above file. Run Prettier t…

f5ab922

…o fix

cloydlau added 2 commits April 21, 2024 15:03

feat: support undefined value for mode property

fcf1a76

chore: code formatting

fde0dc8

chore: move the default value of mode to a common file

69f696a

josdejong added a commit that referenced this pull request Apr 24, 2024

fix: #424 resetting properties to undefined does not restore the defa…

5a153bb

…ult value for the property

josdejong mentioned this pull request Apr 24, 2024

fix: #424 resetting properties to undefined does not restore it's default value #426

Closed

Merge branch 'josdejong:main' into main

e916caf

cloydlau closed this by deleting the head repository Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cloydlau commented Apr 14, 2024

josdejong commented Apr 17, 2024

cloydlau commented Apr 21, 2024

josdejong commented Apr 21, 2024

cloydlau commented Apr 21, 2024

josdejong commented Apr 24, 2024 •

edited

Loading

josdejong commented Apr 24, 2024

cloydlau commented Apr 24, 2024

josdejong commented Apr 24, 2024

josdejong commented Apr 24, 2024

josdejong commented Apr 24, 2024

josdejong commented Apr 24, 2024

SamMousa commented May 16, 2024

josdejong commented May 17, 2024

cloydlau commented May 21, 2024

josdejong commented May 21, 2024

SamMousa commented May 21, 2024

josdejong commented May 21, 2024

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

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

Conversation

cloydlau commented Apr 14, 2024

josdejong commented Apr 17, 2024

cloydlau commented Apr 21, 2024

josdejong commented Apr 21, 2024

cloydlau commented Apr 21, 2024

josdejong commented Apr 24, 2024 • edited Loading

josdejong commented Apr 24, 2024

cloydlau commented Apr 24, 2024

josdejong commented Apr 24, 2024

josdejong commented Apr 24, 2024

josdejong commented Apr 24, 2024

josdejong commented Apr 24, 2024

SamMousa commented May 16, 2024

josdejong commented May 17, 2024

cloydlau commented May 21, 2024

josdejong commented May 21, 2024

SamMousa commented May 21, 2024

josdejong commented May 21, 2024

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

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

josdejong commented Apr 24, 2024 •

edited

Loading