ignore undefined properties

[FredericHeem]

This PR allow properties to be undefined, as we do not always have control on what is given, removing the undefined props from user code would be cumbersome.

[Tao-VanJS]

Thanks for your contribution to the VanJS project! Could you describe the primary use cases of this feature?

[FredericHeem]

In the following simplified example which attempt to implement an input component, the id property may be undefined.

import { css } from "goober";

export default function (context, options = {}) {
  const { theme, van } = context;
  const { palette, shape, shadows } = theme;
  const { div, input, label } = van.tags;
  const style = {
    base: css`
      position: relative;
      display: inline-block;
      font-size: 1rem;
      min-height: 3rem;
    `,
  };

  return (props) => {
    const {
      name,
      id,
      disabled,
      label: labelValue = "",
      error = "",
      ...otherProps
    } = props;
    return div(
      {
        class: `${style.base}`,
      },
      input({
        id,
        name,
        type: "text",
        required: true,
        disabled,
        ...otherProps,
      }),
      label({ htmlFor: id }, labelValue)
    );
  };
}

[Tao-VanJS]

I'm not fully onboard with adding this complexity to VanJS library for this feature.

For the use case you described, there is a relatively simple workaround:

input({
  id ?? "",  // `id` can be undefined
  name,
  type: "text",
  required: true,
  disabled,
  ...otherProps,
})

which can serve your need despite being slightly more verbose. This is indeed similar to #16.

I have to admit that compared to other users in VanJS community, I might value much more the simplicity of library and the symbolic value of being the smallest reactive UI framework in the world. Probably this is because I have put so much heart and soul into optimizing the bundle size, byte-by-byte, thus I might be picky about the additions to the core library. That being said, I might change my mind if I see very compelling reason about the usefulness of this feature.

[FredericHeem]

On one hand it add a few bytes on the lib, on the other side, it adds more code and hassle on the client's code, making the overall bundle size bigger.
I was bitten by the same issue #16, allowing null or undefined in children makes our code much simple and safer.

[Tao-VanJS]

Hmm..., the overall bundle size is a good argument. Let me assess the impact on the bundle size to see if I want to include it in future releases.

One of the complexity of property value is: undefined should be ignored but null can be a valid value. This might be confusing the users.

[Tao-VanJS]

After thinking through this feature and some related issues, my current thought is probably it's better to leave the current behavior as-is. The problem of this feature is:

For DOM node, the value of a property can be a primitive, a state, or a state-derived property. If we define that the property should be ignored for a primitive undefined, what should be the behavior for a state-valued property whose val is undefined? For the sake of consistency, a property value with a state whose val is undefined would mean the property shouldn't exist. Thus let's consider the example below:

const id = van.state("1")
const dom = div({id})
id.val = undefined // expecting `id` of `dom` to be deleted. However it's hard to do.

In practice, it's hard to find a consistent way to "delete" a property of a DOM element. delete dom.id wouldn't work as the statement will simply be ignored in DOM. dom.removeAttribute("id") would work for this case but there are certain properties dom.setAttribute(...) and dom.removeAttribute(...) are not the right behavior, such as value or checked properties.

I guess for your use case, maybe the best way is to provide a default value for id:

const {
  name,
  id = "",
  disabled,
  label: labelValue = "",
  error = "",
  ...otherProps
} = props;

[FredericHeem]

At the moment, passing a null or an undefined property value results in a crash, IMHO, this is not acceptable as a library must be bullet proof whatever the input.

[Tao-VanJS]

I am not aware not any scenario where VanJS could crash the web page (if you have an example, please let me know). If by "crash", you mean an error being thrown, I think that is a common practice among JS frameworks. For instance, the following jQuery code will result an error being thrown:

$('div').click("unknown")

Additionally, if you're using van-{VERSION}.debug.js, you will get a very readable error message when the error is thrown:

Invalid property value for id: Only string, number, boolean, bigint are valid prop value types

Using van-{VERSION}.debug.js is recommended during development whenever an error or anything unexpected occurs.

Hope that addresses your concerns.

[FredericHeem]

By crash, I meant a blank screen, which happened for both #39 and #40

[Tao-VanJS]

If an exception happens before the rendering process, which means none of rendering code will be executed, the blank screen is working as intended, which is not a crash.

Per current version of VanJS, undefined property value is not supported. This is documented in the API reference. A type error will arise if you're using TypeScript, and an error message will pop up right upon using it if van-{VERSION}.debug.js is being used.

Right now I'm quite open to extend the VanJS API to support undefined property value. My current struggle is how to handle state and non-state objects consistently. If you discover a good way to handle it, please let me know. That will effectively unblock this feature.

[FredericHeem]

The blank screen happens for these 2 use cases:

div({ a: undefined })
div(undefined)

The stack trace is:

van.js:54 Uncaught TypeError: Cannot convert undefined or null to object
    at getPrototypeOf (<anonymous>)
    at van.js:54:9
    at Array.forEach (<anonymous>)
    at van.js:52:22
    at App (main.js?t=1685653953858:28:5)
    at main.js?t=1685653953858:32:21

A example of such code can be found at https://github.com/FredericHeem/van-playground

[FredericHeem]

A third way to get a blank screen in the following condition:

const inputState = van.state(undefined);
and 
p({ id: inputState })

[Tao-VanJS]

Thanks @FredericHeem for providing the examples!

I would like to summarize the current status of the discussion. In general 2 issues are being discussed in this thread:

1. Whether VanJS should support undefined as property value in tag function?

For this question, my general feeling is, as mentioned in #39 (comment), there should be more benefit than drawback to support undefined property values. It makes users' life easier and will most-likely generate a smaller overall bundle side (combining VanJS code and users' client code).

On the other hand, the main blocker of implementing this feature is what was discussed in #39 (comment). That is, generally the behavior of state-valued property and primitive-valued property should be consistent. If we define undefined property value as the absence of the property, for the sake of consistency, changing the val of a state-valued property to undefined should have the same effect of constructing a DOM element with undefined as the property value. In other words, the code below:

const id = van.state("some_id")
van.add(document.body, div({id}))
id.val = undefined

should produce the same result as:

van.add(document.body, div({id: undefined}))

but based on the current implementation of this PR, the first piece of code will produce <div></div> while the second piece of code will produce <div id="undefined"></div>.

Based on my current understanding of the DOM model, it seems hard to align the behavior between undefined-valued property and state-valued property whose val has been changed to undefined. This is because in DOM, I haven't found a universal way to delete a property. For id property, probably a good way is assigning an empty string to the id property (or calling removeAttribute("id")). But for on... event handlers, the suitable way is assigning null to the on... property. So this is essentially the blocker of this PR. If we can find a good way to resolve the blocker, it will effectively unblock this PR.

2. Given the current version of VanJS, where undefined is not supported as a valid property value, is the current behavior in VanJS (which throws an exception upon calling to tag function) appropriate?

IMHO, I believe the behavior in VanJS is appropriate. A library can define the interface of its API, which defines what contract the client code needs to follow. Since undefined property value is not supported in the current version of VanJS, calling a tag function with undefined property is undefined behavior by definition, and the library can choose the behavior that is convenient. Indeed, the current behavior in VanJS (throwing an exception) is not a bad one. It lets users know that they're not using the API in a right way at the earliest point, rather than silencing the buggy code. Since usually tag functions are called before the DOM elements being added to document tree via van.add, throwing exceptions in tag functions usually means no change will be reflected in the DOM tree, which means having a blank screen is an intended behavior if the exception is thrown at the page loading time.

On the other hand, VanJS has tried its best to instruct its users that undefined property is not supported in the current version. The API reference of tag function clearly defines what can be accepted as property values, where undefined is not one of them. Users will get type errors if they work with VanJS in TypeScript. And van-{VERSION}.debug.js will print an unambiguous error message to highlight the problem whenever the tag function is being called this way.

[Tao-VanJS]

With the 1.0.0 release, div({id: undefined}, "Text") will produce <div id="undefined">Text</div> instead of throwing an error.

[btakita]

I filed vanjs-org/mini-van#6 & will move my comments here.

The types should allow undefined even if undefined is ignored in the implementation. If a component has an optional prop, then it's value can be undefined.

function MyComponent({ id, text }: { id?: string, text: string }) {
  //  TS2345 ERROR!
  return div({ id, class: 'MyComponent' }, text)
}

With the current types, I'm forced to work around the type check.

function MyComponent({ id, text }: { id?: string, text: string }) {
  return div({ ...(id === undefined ? {} : { id }), class: 'MyComponent' }, text)
}

I have an open PR to fix the type check.

--- Edit

I read the previous conversation in detail. Could we just exclude undefined values in the implementation?

[Tao-VanJS]

Thanks @btakita for your remark!

I think there could be personal opinions regarding this issue. I think there are multiple things to trade-off:

1. The consistent behavior regarding undefined. Ideally:

div({id: undefined})

const id = van.state(undefined)
div({id})

and

const id = van.state("id")
div({id})
id.val = "undefined"

should produce the same result.

2. The little bit convenience on the user code. i.e.: the ability to write

div({id})

instead of

div({...(id ? {id} : {})})

My personal opinion is: 1 is more important than 2. Thus without a good solution on 1, it's probably better to settle on a few more characters of code to handle the absence of the properties.

[btakita]

Perhaps a helper function would be a way to get around the impasse?

function cleanProps(props:Object) {
  for (const key of Object.keys(props)) {
    if (props[key] === undefined) delete props[key]
  }
  return props
}

[Tao-VanJS]

Perhaps a helper function would be a way to get around the impasse?

function cleanProps(props:Object) {
  for (const key of Object.keys(props)) {
    if (props[key] === undefined) props.delete(key)
  }
  return props
}

I am open to this. It's unclear to me at the moment whether it's worthwhile to add it into the official VanJS or VanX package. The function is quite easy to implement on the user side for whoever wants it. I will wait to see if there a universal need from the public for this helper function before deciding whether to include it in official packages.

[btakita]

Sounds good. At least we have a viable solution posted. I'm probably going to use a wrapper Proxy around the van object with various helpers for now.