Skip to content
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

Support spreading null or undefined props #958

Closed
tienpv222 opened this issue Apr 27, 2022 · 5 comments · Fixed by #963
Closed

Support spreading null or undefined props #958

tienpv222 opened this issue Apr 27, 2022 · 5 comments · Fixed by #963
Labels
enhancement New feature or request
Milestone

Comments

@tienpv222
Copy link

tienpv222 commented Apr 27, 2022

Describe the bug

Spreading null or undefined props gives runtime errors.

While it is not exactly the same, such situation is usually supported in places like plain object or React. It might be nice if Solid provides similar behavior.

Additional consideration: support full native spreading JS object with boolean, number, etc. ?

Your Example Website or App

N/A

Steps to Reproduce the Bug or Issue

N/A

Expected behavior

Spreading null or undefined props shouldn't throw exceptions.

Screenshots or Videos

No response

Platform

N/A

Additional context

Related discussions:

@lxsmnsyc
Copy link
Member

Doing {...(nullable ?? {})} should work, but I find it a bit inconsistent with native behavior. However that's just my personal thought.

Now I found this weird. I've been using TS for more than 3 years and I just learned this just now (since TS wouldn't allow you do this). I'm okay with supporting this but this just kind of felt weird for me 😂

@edemaine
Copy link
Contributor

edemaine commented Apr 28, 2022

Maybe you've been using TS too much. 😉 It's part of ECMAScript (also in Object.assign), and quite useful when you have optional arguments (e.g. (options) => { options = {...defaults, ...options}; ... }). Indeed, this use-case is supported in TypeScript.

In the context of Solid, suppose you have an optional prop props.divProps that can be an object of props to pass in to a child component (useful when you have more than one such child component that you want to customize). It's natural to want to write <div {...props.divProps}>, which gets ignored when props.divProps is null, just like you can currently write <div foo={foo()}> which gets ignored when foo() is null.

@ryansolid ryansolid added the enhancement New feature or request label Apr 28, 2022
@ryansolid ryansolid added this to the 1.4.0 milestone May 1, 2022
ryansolid added a commit that referenced this issue May 1, 2022
@tienpv222
Copy link
Author

tienpv222 commented May 2, 2022

@ryansolid Please excuse me if I'm misunderstanding. I think 5292c2d didn't handle spreading component props.

@lxsmnsyc
Copy link
Member

lxsmnsyc commented May 2, 2022

@tienpv222 the commit was made in the compiler: ryansolid/dom-expressions@a8fc08c

otonashixav added a commit to otonashixav/solid that referenced this issue May 2, 2022
- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes solidjs#958
otonashixav added a commit to otonashixav/solid that referenced this issue May 2, 2022
- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes solidjs#958
@ryansolid
Copy link
Member

Yeah i missed looking at mergeProps in Solid. We can add additional checks.

otonashixav added a commit to otonashixav/solid that referenced this issue May 3, 2022
- `createComponent` passes an empty object instead of nullable props which are spread
- `mergeProps` treats nullable sources as empty objects
- added tests

fixes solidjs#958
otonashixav added a commit to otonashixav/solid that referenced this issue May 3, 2022
- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes solidjs#958
otonashixav added a commit to otonashixav/solid that referenced this issue May 3, 2022
- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes solidjs#958
ryansolid pushed a commit that referenced this issue May 3, 2022
- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes #958
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants