-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ignore undefined properties #39
base: main
Are you sure you want to change the base?
Conversation
Thanks for your contribution to the VanJS project! Could you describe the primary use cases of this feature? |
In the following simplified example which attempt to implement an input component, the id property may be undefined.
|
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. |
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. |
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: |
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 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. I guess for your use case, maybe the best way is to provide a default value for const {
name,
id = "",
disabled,
label: labelValue = "",
error = "",
...otherProps
} = props; |
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. |
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
Using Hope that addresses your concerns. |
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, Right now I'm quite open to extend the VanJS API to support |
The blank screen happens for these 2 use cases:
The stack trace is:
A example of such code can be found at https://github.com/FredericHeem/van-playground |
A third way to get a blank screen in the following condition:
|
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
|
With the |
I filed vanjs-org/mini-van#6 & will move my comments here. The types should allow 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 |
Thanks @btakita for your remark! I think there could be personal opinions regarding this issue. I think there are multiple things to trade-off:
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.
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. |
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
} |
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. |
Sounds good. At least we have a viable solution posted. I'm probably going to use a wrapper |
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.