-
Notifications
You must be signed in to change notification settings - Fork 78
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
Typed children for function-based widgets #544
Conversation
…ccept it being passed from .childre()
src/core/interfaces.d.ts
Outdated
@@ -4,6 +4,10 @@ import Map from '../shim/Map'; | |||
import WeakMap from '../shim/WeakMap'; | |||
import { RegistryHandler } from './RegistryHandler'; | |||
|
|||
export type OmitProp<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>; | |||
export type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this conflict with the global Omit
type added in 3.5? I presume not but worth a check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't do, I did a quick check and there no complaints about Omit
src/core/vdom.ts
Outdated
): WNode<W> { | ||
if ((properties as any).children) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct now? would it not be __children__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and a couple of other misses too.
@@ -3430,6 +3430,42 @@ jsdomDescribe('vdom', () => { | |||
assert.isTrue(consoleWarnStub.calledOnce); | |||
}); | |||
|
|||
it('typed children', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need an example test with w()
also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
@@ -70,7 +70,7 @@ describe('selector', () => { | |||
assert.deepEqual(adapter.getChildren(v('div', {}, [w(WidgetBase, {})])), [w(WidgetBase, {})]); | |||
assert.deepEqual(adapter.getChildren(w(WidgetBase, {}, [v('span')])), [v('span')]); | |||
assert.deepEqual(adapter.getChildren(w(WidgetBase, {}, [w(WidgetBase, {})])), [w(WidgetBase, {})]); | |||
assert.deepEqual(adapter.getChildren(w(WidgetBase, {})), []); | |||
assert.deepEqual(adapter.getChildren(w(WidgetBase, {})), undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come this had to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because by default children are not set to an empty array when using w
any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this is the assertion for getChildren
no? which is both typed as an array and returns an empty array if not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is a WNode or VNode then it just returns the children from the element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, sorry I missed that this was in the selector, and not the assertion template. This makes me concerned about these types/values a little bit though.
I think any kind of children
interface should be consistently returning either an empty array []
for no children, an array of one ['a']
for one item, or an array of many ['a','b','c']
for many items. This means the end user can consistently work on an array without guards for api's such as this, and would match other child like api's such as NodeList
.
I'm not sure what the ramifications are on types or whether it's possible with TSX
, but it'd seem more consistent to make the user define create().children<[ whatever ]>()
over create().children<whatever>()
if they need to define a single item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fair point about always having children and it being an array (that can be defaulted). I've made some changes (WIP atm as need to put back the defaulting of children) that added support for children to be mandatory and able to be typed singularly but are consumed by the widget author as the first item in an array.
@@ -39,12 +39,12 @@ export function createIntersectionMock() { | |||
} | |||
}; | |||
|
|||
function mockIntersection(): MiddlewareResult<any, any, any>; | |||
function mockIntersection(): MiddlewareResult<any, any, any, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a convenience type for this? it's used quite a lot in the mocks, and presumably would be also for a user writing a mock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a DefaultMiddlewareResult
interface that represents MiddlewareResult<any, any, any, any>
this is awesome 👍 took some wrangling in the end but worth it. |
Type: feature
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
Provides extended typing of children for function-based widgets, supported by all widget authoring patterns. When children types are defined it makes them mandatory for widget usage.
Typed children provide a first class replacement for widgets that use render props, for example
Outlet
.Resolves #528
Resolves #43