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

Flip the arguments of Blocks and make the query optional #18374

Merged
merged 2 commits into from Mar 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-client/src/ReactFlightClient.js
Expand Up @@ -152,7 +152,7 @@ export function reportGlobalError(response: Response, error: Error): void {
}

function readMaybeChunk<T>(maybeChunk: Chunk<T> | T): T {
if ((maybeChunk: any).$$typeof !== CHUNK_TYPE) {
if (maybeChunk == null || (maybeChunk: any).$$typeof !== CHUNK_TYPE) {
// $FlowFixMe
return maybeChunk;
}
Expand Down
41 changes: 34 additions & 7 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Expand Up @@ -29,12 +29,15 @@ describe('ReactFlight', () => {
act = ReactNoop.act;
});

function block(query, render) {
function block(render, load) {
return function(...args) {
let curriedQuery = () => {
return query(...args);
if (load === undefined) {
return [Symbol.for('react.server.block'), render];
}
let curriedLoad = () => {
return load(...args);
};
return [Symbol.for('react.server.block'), render, curriedQuery];
return [Symbol.for('react.server.block'), render, curriedLoad];
};
}

Expand Down Expand Up @@ -70,8 +73,32 @@ describe('ReactFlight', () => {
});

if (ReactFeatureFlags.enableBlocksAPI) {
it('can transfer a Block to the client and render there', () => {
function Query(firstName, lastName) {
it('can transfer a Block to the client and render there, without data', () => {
function User(props, data) {
return (
<span>
{props.greeting} {typeof data}
</span>
);
}
let loadUser = block(User);
let model = {
User: loadUser('Seb', 'Smith'),
Copy link
Contributor

Choose a reason for hiding this comment

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

the type signature for loadUser here would disallow args, though, right?

};

let transport = ReactNoopFlightServer.render(model);
let root = ReactNoopFlightClient.read(transport);

act(() => {
let UserClient = root.model.User;
ReactNoop.render(<UserClient greeting="Hello" />);
});

expect(ReactNoop).toMatchRenderedOutput(<span>Hello undefined</span>);
});

it('can transfer a Block to the client and render there, with data', () => {
function load(firstName, lastName) {
return {name: firstName + ' ' + lastName};
}
function User(props, data) {
Expand All @@ -81,7 +108,7 @@ describe('ReactFlight', () => {
</span>
);
}
let loadUser = block(Query, User);
let loadUser = block(User, load);
let model = {
User: loadUser('Seb', 'Smith'),
};
Expand Down
Expand Up @@ -44,12 +44,15 @@ describe('ReactFlightDOMRelay', () => {
return model;
}

function block(query, render) {
function block(render, load) {
return function(...args) {
let curriedQuery = () => {
return query(...args);
if (load === undefined) {
return [Symbol.for('react.server.block'), render];
}
let curriedLoad = () => {
return load(...args);
};
return [Symbol.for('react.server.block'), render, curriedQuery];
return [Symbol.for('react.server.block'), render, curriedLoad];
};
}

Expand Down Expand Up @@ -89,7 +92,7 @@ describe('ReactFlightDOMRelay', () => {
});

it.experimental('can transfer a Block to the client and render there', () => {
function Query(firstName, lastName) {
function load(firstName, lastName) {
return {name: firstName + ' ' + lastName};
}
function User(props, data) {
Expand All @@ -99,7 +102,7 @@ describe('ReactFlightDOMRelay', () => {
</span>
);
}
let loadUser = block(Query, User);
let loadUser = block(User, load);
let model = {
User: loadUser('Seb', 'Smith'),
};
Expand Down
Expand Up @@ -62,7 +62,7 @@ describe('ReactFlightDOM', () => {
};
}

function block(query, render) {
function block(render, load) {
let idx = webpackModuleIdx++;
webpackModules[idx] = {
d: render,
Expand All @@ -73,10 +73,13 @@ describe('ReactFlightDOM', () => {
name: 'd',
};
return function(...args) {
let curriedQuery = () => {
return query(...args);
if (load === undefined) {
return [Symbol.for('react.server.block'), render];
}
let curriedLoad = () => {
return load(...args);
};
return [Symbol.for('react.server.block'), 'path/' + idx, curriedQuery];
return [Symbol.for('react.server.block'), 'path/' + idx, curriedLoad];
};
}

Expand Down Expand Up @@ -288,7 +291,7 @@ describe('ReactFlightDOM', () => {
reject(e);
};
});
function Query() {
function load() {
if (promise) {
throw promise;
}
Expand All @@ -300,8 +303,8 @@ describe('ReactFlightDOM', () => {
function DelayedText({children}, data) {
return <Text>{children}</Text>;
}
let _block = block(Query, DelayedText);
return [_block(), _resolve, _reject];
let loadBlock = block(DelayedText, load);
return [loadBlock(), _resolve, _reject];
}

const [FriendsModel, resolveFriendsModel] = makeDelayedText();
Expand Down
40 changes: 31 additions & 9 deletions packages/react-reconciler/src/__tests__/ReactBlocks-test.js
Expand Up @@ -49,8 +49,30 @@ describe('ReactBlocks', () => {
};
});

it.experimental('renders a simple component', () => {
function User(props, data) {
return <div>{typeof data}</div>;
}

function App({Component}) {
return (
<Suspense fallback={'Loading...'}>
<Component name="Name" />
</Suspense>
);
}

let loadUser = block(User);

ReactNoop.act(() => {
ReactNoop.render(<App Component={loadUser()} />);
});

expect(ReactNoop).toMatchRenderedOutput(<div>undefined</div>);
});

it.experimental('prints the name of the render function in warnings', () => {
function Query(firstName) {
function load(firstName) {
return {
name: firstName,
};
Expand All @@ -69,7 +91,7 @@ describe('ReactBlocks', () => {
);
}

let loadUser = block(Query, User);
let loadUser = block(User, load);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunate that the order gets reversed from the grammar. User + load = loadUser.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's just a naming convention that we could change, though. It's probably helpful to use different names for the result of block, since calling loadUser here doesn't directly call load.


expect(() => {
ReactNoop.act(() => {
Expand All @@ -86,8 +108,8 @@ describe('ReactBlocks', () => {
);
});

it.experimental('renders a component with a suspending query', async () => {
function Query(id) {
it.experimental('renders a component with a suspending load', async () => {
function load(id) {
return {
id: id,
name: readString('Sebastian'),
Expand All @@ -102,7 +124,7 @@ describe('ReactBlocks', () => {
);
}

let loadUser = block(Query, Render);
let loadUser = block(Render, load);

function App({User}) {
return (
Expand All @@ -128,7 +150,7 @@ describe('ReactBlocks', () => {
it.experimental(
'does not support a lazy wrapper around a chunk',
async () => {
function Query(id) {
function load(id) {
return {
id: id,
name: readString('Sebastian'),
Expand All @@ -143,7 +165,7 @@ describe('ReactBlocks', () => {
);
}

let loadUser = block(Query, Render);
let loadUser = block(Render, load);

function App({User}) {
return (
Expand Down Expand Up @@ -187,7 +209,7 @@ describe('ReactBlocks', () => {
it.experimental(
'can receive updated data for the same component',
async () => {
function Query(firstName) {
function load(firstName) {
return {
name: firstName,
};
Expand All @@ -203,7 +225,7 @@ describe('ReactBlocks', () => {
);
}

let loadUser = block(Query, Render);
let loadUser = block(Render, load);

function App({User}) {
return (
Expand Down
12 changes: 6 additions & 6 deletions packages/react-server/src/ReactFlightServer.js
Expand Up @@ -191,11 +191,11 @@ export function resolveModelToJSON(
}
}
case '2': {
// Query
let query: () => ReactModel = (value: any);
// Load function
let load: () => ReactModel = (value: any);
try {
// Attempt to resolve the query.
return query();
// Attempt to resolve the data.
return load();
} catch (x) {
if (
typeof x === 'object' &&
Expand All @@ -204,12 +204,12 @@ export function resolveModelToJSON(
) {
// Something suspended, we'll need to create a new segment and resolve it later.
request.pendingChunks++;
let newSegment = createSegment(request, query);
let newSegment = createSegment(request, load);
let ping = newSegment.ping;
x.then(ping, ping);
return serializeIDRef(newSegment.id);
} else {
// This query failed, encode the error as a separate row and reference that.
// This load failed, encode the error as a separate row and reference that.
request.pendingChunks++;
let errorId = request.nextChunkId++;
emitErrorChunk(request, errorId, x);
Expand Down
33 changes: 25 additions & 8 deletions packages/react/src/ReactBlock.js
Expand Up @@ -16,14 +16,14 @@ import {
REACT_FORWARD_REF_TYPE,
} from 'shared/ReactSymbols';

type BlockQueryFunction<Args: Iterable<any>, Data> = (...args: Args) => Data;
type BlockLoadFunction<Args: Iterable<any>, Data> = (...args: Args) => Data;
export type BlockRenderFunction<Props, Data> = (
props: Props,
data: Data,
) => React$Node;

type Payload<Props, Args: Iterable<any>, Data> = {
query: BlockQueryFunction<Args, Data>,
load: BlockLoadFunction<Args, Data>,
args: Args,
render: BlockRenderFunction<Props, Data>,
};
Expand All @@ -44,20 +44,20 @@ function lazyInitializer<Props, Args: Iterable<any>, Data>(
): BlockComponent<Props, Data> {
return {
$$typeof: REACT_BLOCK_TYPE,
_data: payload.query.apply(null, payload.args),
_data: payload.load.apply(null, payload.args),
_render: payload.render,
};
}

export function block<Args: Iterable<any>, Props, Data>(
query: BlockQueryFunction<Args, Data>,
render: BlockRenderFunction<Props, Data>,
load?: BlockLoadFunction<Args, Data>,
): (...args: Args) => Block<Props> {
if (__DEV__) {
if (typeof query !== 'function') {
if (load !== undefined && typeof load !== 'function') {
console.error(
'Blocks require a query function but was given %s.',
query === null ? 'null' : typeof query,
'Blocks require a load function, if provided, but was given %s.',
load === null ? 'null' : typeof load,
);
}
if (render != null && render.$$typeof === REACT_MEMO_TYPE) {
Expand Down Expand Up @@ -97,11 +97,28 @@ export function block<Args: Iterable<any>, Props, Data>(
}
}

if (load === undefined) {
return function(): Block<Props> {
let blockComponent: BlockComponent<Props, void> = {
$$typeof: REACT_BLOCK_TYPE,
_data: undefined,
// $FlowFixMe: Data must be void in this scenario.
_render: render,
};

// $FlowFixMe
return blockComponent;
};
}

// Trick to let Flow refine this.
let loadFn = load;

return function(): Block<Props> {
let args: Args = arguments;

let payload: Payload<Props, Args, Data> = {
query: query,
load: loadFn,
args: args,
render: render,
};
Expand Down