Skip to content

Conversation

@TrySound
Copy link
Collaborator

@TrySound TrySound commented Feb 14, 2018

Would be good to release this fixes

@renatorib
Copy link
Owner

renatorib commented Feb 15, 2018

Great!

Could you type the compose function too? It's the only one missing I think.

Just type that it is a function that receives components and returns another component. The rest is impossible to type

@TrySound
Copy link
Collaborator Author

@renatorib I already said it's not typable. I can only make it any.

@renatorib
Copy link
Owner

renatorib commented Feb 15, 2018

I know it's impossible to type. But we can do it generically

In typescript I think it's something like this:

type ComposeRender = (...props: any[]) => ReactNode
type ComposeInput = ComponentType<any> | ReactElement<any>

declare function compose(...inputs: ComposeInput[]): ComponentType<{ render: ComposeRender }>
declare function compose(...inputs: ComposeInput[]): ComponentType<{ children: ComposeRender }>

@TrySound
Copy link
Collaborator Author

Well, any. Unsafe. But ok.

@renatorib
Copy link
Owner

Well, I guess it's better than nothing. 😅
At least we can ensure that people are using the function correctly.

@TrySound
Copy link
Collaborator Author

@renatorib There is a problem. render or children are not optional. We can't omit them when create component.

@renatorib
Copy link
Owner

Humm. Got it.
It's possible to do it with overloads?

@TrySound
Copy link
Collaborator Author

What do you mean?

@renatorib
Copy link
Owner

It's just a question. I don't know how overloading works. It's possible to solve this problem with overloads?

@renatorib
Copy link
Owner

Anyway. I'll merge this. I'll see what I can do with the compose in another PR. What do you think?

@TrySound
Copy link
Collaborator Author

We already use overload for each component. The first one works with children and the second one with render props.

@renatorib
Copy link
Owner

So we can't do this with compose too?

@TrySound
Copy link
Collaborator Author

The problem is not compose itself. The problem is components which can't be created without one prop and passed to compose. So nothing can be used with compose.

@renatorib
Copy link
Owner

renatorib commented Feb 15, 2018

Oh. Got it. Sorry.
So render/children are optional, it's not?

@TrySound
Copy link
Collaborator Author

TrySound commented Feb 15, 2018

Compose requires them to be optional. But it's not type safe with regular use. So currently components require one of children or render to be passed.

@TrySound
Copy link
Collaborator Author

Can we release this and maybe discuss compose later? It always can be released as feature.

@renatorib renatorib merged commit 8a62567 into renatorib:master Feb 15, 2018
@renatorib
Copy link
Owner

renatorib commented Feb 15, 2018

Alright! Thanks 😄
I'll publish it soon

@TrySound TrySound deleted the fix-types branch February 15, 2018 21:25
@TrySound
Copy link
Collaborator Author

Friendly ping :)

@TrySound
Copy link
Collaborator Author

Would be good if you will let me publish such fixes.

@renatorib
Copy link
Owner

@TrySound published v1.0.0-alpha.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants