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

Typed children for function-based widgets #544

Merged
merged 16 commits into from
Oct 1, 2019

Conversation

agubler
Copy link
Member

@agubler agubler commented Sep 29, 2019

Type: feature

The following has been addressed in the PR:

Description:

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.

import { create, tsx } from '@dojo/framework/core/vdom';

const factory = create().children<(value: string) => RenderResult>();

const Foo = factory(function Foo({ children }) {
    const [childRender] = children();
    return childRender('text');
});

// usages

// using TSX
<Foo>{(value) => <div>value</div>}</Foo>

// Using the widget function
Foo({}, [(value) => <div>value</div>]); 

// Using the `w()` pragma
w(Foo, {}, [(value) => <div>value</div>]);

Typed children provide a first class replacement for widgets that use render props, for example Outlet.

<Outlet id="outlet" renderer={(matchDetails) => {
    if (matchDetails.isExact()) {
        return <Home />;
    }
    return null;
}}/>

<Outlet id="outlet">{(matchDetails) => {
    if (matchDetails.isExact()) {
        return <Home />;
    }
    return null;
}}</Outlet>

Resolves #528
Resolves #43

@agubler agubler added enhancement New feature or request breaking change Indicates the issue/pull request would result in a breaking change next Issue/Pull Request for the next major version area: core Core labels Sep 29, 2019
@@ -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>>;
Copy link
Contributor

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

Copy link
Member Author

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) {
Copy link
Contributor

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__?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

Copy link
Member Author

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', () => {
Copy link
Contributor

@matt-gadd matt-gadd Sep 30, 2019

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that

Copy link
Member Author

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);
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@matt-gadd matt-gadd Sep 30, 2019

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.

Copy link
Member Author

@agubler agubler Sep 30, 2019

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>;
Copy link
Contributor

@matt-gadd matt-gadd Sep 30, 2019

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?

Copy link
Member Author

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>

@matt-gadd
Copy link
Contributor

this is awesome 👍 took some wrangling in the end but worth it.

@agubler agubler merged commit 47a39b4 into dojo:master Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Core breaking change Indicates the issue/pull request would result in a breaking change enhancement New feature or request next Issue/Pull Request for the next major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support named children Support typed children in TSX
2 participants