Skip to content

fix(#2644): adapt the react children api #2645

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix(#2644): adapt the react children api #2645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
867fd81
fix(#2644): adapt the react children api
deleonio Jul 22, 2020
bda3f94
docs: add type annotation for d.ts
deleonio Jul 22, 2020
b1f4604
fix type annotation
deleonio Jul 22, 2020
e5a3a86
adapt tests
deleonio Jul 22, 2020
cd92f41
fix the context handling / this
deleonio Jul 28, 2020
a507b84
test: revert the changes
deleonio Jul 28, 2020
9b6fda2
Merge branch 'master' into fix/#2644
Jul 28, 2020
8367c16
add a test
deleonio Jul 28, 2020
8265ed8
Merge branch 'master' into fix/#2644
deleonio Sep 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions compat/src/Children.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { toChildArray } from 'preact';

const mapFn = (children, fn) => {
if (children == null) return null;
return toChildArray(toChildArray(children).map(fn));
const mapFn = (children, fn, context) => {
if (!children) return null;
return toChildArray(children).reduce(
Copy link
Member

Choose a reason for hiding this comment

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

We don't generally use reduce(). I'd prefer to use .map(fn.bind(ctx)) for size and perf reasons.

(acc, value, index) => acc.concat(fn.call(context, value, index)),
[]
);
};

// This API is completely unnecessary for Preact, so it's basically passthrough.
Expand Down
10 changes: 6 additions & 4 deletions compat/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as _hooks from '../../hooks';
import * as preact from '../../src';
import { JSXInternal } from '../../src/jsx';
import * as _Suspense from './suspense';
import * as _SuspenseList from './suspense-list'
import * as _SuspenseList from './suspense-list';

// export default React;
export = React;
Expand Down Expand Up @@ -43,7 +43,7 @@ declare namespace React {
// Suspense
export import Suspense = _Suspense.Suspense;
export import lazy = _Suspense.lazy;
export import SuspenseList = _SuspenseList.SuspenseList
export import SuspenseList = _SuspenseList.SuspenseList;

// Compat
export import StrictMode = preact.Fragment;
Expand Down Expand Up @@ -111,11 +111,13 @@ declare namespace React {
export const Children: {
map<T extends preact.ComponentChild, R>(
children: T | T[],
fn: (child: T, i: number) => R
fn: (this: any, child: T, i: number) => R,
context: any
): R[];
forEach<T extends preact.ComponentChild>(
children: T | T[],
fn: (child: T, i: number) => void
fn: (this: any, child: T, i: number) => void,
context: any
): void;
count: (children: preact.ComponentChildren) => number;
only: (children: preact.ComponentChildren) => preact.ComponentChild;
Expand Down
25 changes: 14 additions & 11 deletions compat/test/browser/Children.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ describe('Children', () => {

it('should throw if no children are passed', () => {
// eslint-disable-next-line prefer-arrow-callback
expect(function() {
expect(function () {
render(<Foo />, scratch);
}).to.throw();
});

it('should throw if more children are passed', () => {
// eslint-disable-next-line prefer-arrow-callback
expect(function() {
expect(function () {
render(
<Foo>
foo
Expand All @@ -77,7 +77,7 @@ describe('Children', () => {

describe('.map', () => {
function Foo(props) {
let children = Children.map(props.children, child => (
let children = Children.map(props.children, (child) => (
<span>{child}</span>
));
return <div>{children}</div>;
Expand All @@ -99,17 +99,20 @@ describe('Children', () => {
expect(serializeHtml(scratch)).to.equal('<div></div>');
});

it('should work with children as zero number', () => {
const testNumber = 0;

render(<Foo>{testNumber}</Foo>, scratch);
expect(serializeHtml(scratch)).to.equal('<div><span>0</span></div>');
it('should propagate the this context', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a great start, but I think the commit isn't complete. The test defines a few functions but never calls them anywhere. This will make the test always pass, even against current master and without the changes in this PR.

Suggested change
it('should propagate the this context', () => {
it('should propagate this context', () => {
const context = {};
const spy = sinon.spy(child => child); // noop
const Foo = ({ children }) => {
return React.Children.map(children, spy, context);
};
render(<Foo>foo</Foo>, scratch);
expect(spy.thisValues[0]).to.equal(context);
});

const context = {};
const fn = () => {
expect(this).to.equal(context);
};
({ children }) => {
React.Children.map(children, fn, (context) => {});
};
});

it('should flatten result', () => {
const ProblemChild = ({ children }) => {
return React.Children.map(children, child => {
return React.Children.map(child.props.children, x => x);
return React.Children.map(children, (child) => {
return React.Children.map(child.props.children, (x) => x);
}).filter(React.isValidElement);
};

Expand Down Expand Up @@ -165,7 +168,7 @@ describe('Children', () => {
describe('.forEach', () => {
function Foo(props) {
let children = [];
Children.forEach(props.children, child =>
Children.forEach(props.children, (child) =>
children.push(<span>{child}</span>)
);
return <div>{children}</div>;
Expand Down