Allow nested bindings

[apedroferreira]

Allow for nested bindings, which is needed for example to have the chart component work as we intend to (with nested data series that can be bound to different datasources).

Extracted from #2081

Tried many different approaches, this one:

• works universally for nested bindings inside objects or arrays
• does not add anything new to the schema, we can just nest bindings in the yaml files

Open for feedback of course!
Also not sure if I might have missed some edge cases or possible bugs with the parts of the code I altered.

[apedroferreira]

The preview/production runtime seem to turn all $$jsExpression DOM bindings into $jsExpression...
So as I've done before for refs, it seems that using $jsExpression instead of $$jsExpression works better... I'm proposing that change here with a migration in pages from API v1 from v2, feel free to provide feedback.

[Janpot]

Fixing the double dollar problem in #2165. We do need to think about a good migration strategy going forward, but I'm not sure we should mix that in here if we don't have to, there's a lot to consider.

[apedroferreira]

I've added changing bindings from

{
  type: 'jsExpression'
  value: string
}

to

{
  $$jsExpression: string
}

as proposed. Also applies to $$env, $$jsExpressionAction, $$navigationAction, $$secret (do we use this?) and no longer uses a const type, just a direct value.

It's a lot of changes, hopefully nothing else is broken, I could not find anything else so far.

Choices made that could deserve some attention/discussion:

• Used as assertions a lot sometimes because I could not find a better way to infer the type of a bindable from the key name. (for example, bindable?.$$jsExpression will show an error if we don't assert the BindableAttrValue type to JsExpressionAttrValue. Maybe there's a better way to do this.)
• To make it simpler and cleaner to determine what kind of bindable is being used or retrieve its value, I've created getBindingValue and getBindingType utilities.
• Added an appDom migration (v7) to address these changes, but because the new bindable types are incompatible with the old ones, made the types in old migrations work by keeping some old types in migrations/types/v7Legacy to be used in old migrations.

[apedroferreira]

As for testing nested bindings we don't really have a use case for nested bindings yet so I think we can cover it with the chart component tests once that component is added.

[Janpot]

This doesn't seem right. page is supposed to be a string according to the schema: https://github.com/mui/mui-toolpad/blob/ae82d58325f769644a7fb0f2f0b6b4683e75d285/packages/toolpad-app/src/server/schema.ts#L58
I'd expect a test to break because of this, I thought Toolpad wouldn't start up when there's an invalid file 🤔

[apedroferreira]

I think the code itself was expecting a ref, anyway using a string seems good - I'll make it a string and take a look at the schema validation.

[apedroferreira]

It's not easy to make the schema validation work here, seems like z.record(jsonSchema) is allowing any object to pass validation, but without it the unions also don't work as expected. So far haven't solved it, I'm looking into it.

[apedroferreira]

ok i finally found a way

[Janpot]

Aah good catch,

One thing we could do is disallow properties in the jsonSchema that start with $$

const literalSchema = z.union([z.string(), z.number(), z.boolean(), z.null()]);
type Literal = z.infer<typeof literalSchema>;
type Json = Literal | { [key: string]: Json } | Json[];
export const jsonSchema: z.ZodType<Json> = z
  .lazy(() =>
    z.union([
      ...literalSchema.options,
      z.array(jsonSchema),
      // disallow $$ properties here
      z.record(jsonSchema).refine((obj) => !Object.keys(obj).some((key) => key.startsWith('$$')))
    ]),
  )

But this would ofcourse not work for nested bindings, so we need to pull back in jsExpressionBindingSchema. But then we should probably not call it jsonSchema anymore. We can probably instead remove jsonSchema from bindablePropSchema and insteadcreate that type like. (You'll have to do some trickery to make recursive types work, see how it's done in jsonSchema)

export const bindablePropSchema = z.lazy(() => z.union([
  ...literalSchema.options,
  z.array(bindablePropSchema),
  z.record(bindablePropSchema).refine((obj) => !Object.keys(obj).some((key) => key.startsWith('$$'))),
  jsExpressionBindingSchema,
  envBindingSchema,
  jsExpressionActionSchema,
  navigationActionSchema,
  templateSchema,
]));

[apedroferreira]

Yep this is exactly what I was already doing, I was just going to try to pass all the possible $$... properties to exclude in the refine function, but we can also just exclude if they start with $$, maybe that's easier to maintain.

[Janpot]

Note: We will remove migrations post 0.1.16.

[apedroferreira]

Ok, the one I added was just to help if there's any scenario where it's needed, but we can remove it later along with all these migrations.

[Janpot]

yass!

[Janpot]

yep, secret is obsolete in post-13

[Janpot]

I'd prefer this written with nested update calls as we do everywhere else. It's slightly more verbose but it's more type-safe, typescript will yell at us if we update the AppDom but forget to update this statement.
It's probably slightly more performant as well and it doesn't create clones if it doesn't need to.

We can always start using immer to simplify these sort of updates ina type safe way.

[apedroferreira]

Got it, I'll try to use update for now.

[apedroferreira]

The reason I created and used an updatePath method is because lodash makes it easy to update an object by passing a string with the path of the property we want to change (it also needs clone to be immutable, though). And in this case we could have a unknown number of nested objects or arrays.

I could create an issue to write our own method for this later using our own update instead of lodash methods? We would need to parse the path manually.

[Janpot]

Looking good, just this one comment seems a blocker to me

[Janpot]

Lovely