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

Add fetch Instrumentation to Dedupe Fetches #25516

Merged
merged 3 commits into from
Oct 19, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"minimist": "^1.2.3",
"mkdirp": "^0.5.1",
"ncp": "^2.0.0",
"@actuallyworks/node-fetch": "^2.6.0",
"pacote": "^10.3.0",
"prettier": "1.19.1",
"prop-types": "^15.6.2",
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,8 @@ if (enableUseMemoCacheHook) {
};
}

function noop(): void {}

function use<T>(usable: Usable<T>): T {
if (usable !== null && typeof usable === 'object') {
// $FlowFixMe[method-unbinding]
Expand All @@ -793,6 +795,11 @@ function use<T>(usable: Usable<T>): T {
index,
);
if (prevThenableAtIndex !== null) {
if (thenable !== prevThenableAtIndex) {
// Avoid an unhandled rejection errors for the Promises that we'll
// intentionally ignore.
thenable.then(noop, noop);
}
switch (prevThenableAtIndex.status) {
case 'fulfilled': {
const fulfilledValue: T = prevThenableAtIndex.value;
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,8 @@ if (enableUseMemoCacheHook) {
};
}

function noop(): void {}

function use<T>(usable: Usable<T>): T {
if (usable !== null && typeof usable === 'object') {
// $FlowFixMe[method-unbinding]
Expand All @@ -793,6 +795,11 @@ function use<T>(usable: Usable<T>): T {
index,
);
if (prevThenableAtIndex !== null) {
if (thenable !== prevThenableAtIndex) {
// Avoid an unhandled rejection errors for the Promises that we'll
// intentionally ignore.
thenable.then(noop, noop);
}
switch (prevThenableAtIndex.status) {
case 'fulfilled': {
const fulfilledValue: T = prevThenableAtIndex.value;
Expand Down
5 changes: 5 additions & 0 deletions packages/react-server/src/ReactFizzHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,11 @@ function use<T>(usable: Usable<T>): T {
index,
);
if (prevThenableAtIndex !== null) {
if (thenable !== prevThenableAtIndex) {
// Avoid an unhandled rejection errors for the Promises that we'll
// intentionally ignore.
thenable.then(noop, noop);
}
switch (prevThenableAtIndex.status) {
case 'fulfilled': {
const fulfilledValue: T = prevThenableAtIndex.value;
Expand Down
15 changes: 14 additions & 1 deletion packages/react-server/src/ReactFlightCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,22 @@

import type {CacheDispatcher} from 'react-reconciler/src/ReactInternalTypes';

function createSignal(): AbortSignal {
return new AbortController().signal;
}

export const DefaultCacheDispatcher: CacheDispatcher = {
getCacheSignal(): AbortSignal {
throw new Error('Not implemented.');
if (!currentCache) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why unroll this? Does this really help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just that I don't want to call the Dispatcher and create a dependency on the object wrapper. But this is going away soon anyway as we need a proper lifetime for it.

throw new Error('Reading the cache is only supported while rendering.');
}
let entry: AbortSignal | void = (currentCache.get(createSignal): any);
if (entry === undefined) {
entry = createSignal();
// $FlowFixMe[incompatible-use] found when upgrading Flow
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I don't get why a FlowFixMe is necessary here. Is it because the createSignal call resets the type refinement? Would a const binding above fix it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like it was added during a codemod to getCacheForType below, and then you copy pasted. Guessing it's easy to fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just temporary until I add the lifetime to Flight next, and getCacheForType will be replaced by cache().

currentCache.set(createSignal, entry);
}
return entry;
},
getCacheForType<T>(resourceType: () => T): T {
if (!currentCache) {
Expand Down
7 changes: 7 additions & 0 deletions packages/react-server/src/ReactFlightHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ function useId(): string {
return ':' + currentRequest.identifierPrefix + 'S' + id.toString(32) + ':';
}

function noop(): void {}

function use<T>(usable: Usable<T>): T {
if (usable !== null && typeof usable === 'object') {
// $FlowFixMe[method-unbinding]
Expand All @@ -147,6 +149,11 @@ function use<T>(usable: Usable<T>): T {
index,
);
if (prevThenableAtIndex !== null) {
if (thenable !== prevThenableAtIndex) {
// Avoid an unhandled rejection errors for the Promises that we'll
// intentionally ignore.
thenable.then(noop, noop);
}
switch (prevThenableAtIndex.status) {
case 'fulfilled': {
const fulfilledValue: T = prevThenableAtIndex.value;
Expand Down
3 changes: 3 additions & 0 deletions packages/react/src/React.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ import ReactSharedInternals from './ReactSharedInternals';
import {startTransition} from './ReactStartTransition';
import {act} from './ReactAct';

// Patch fetch
import './ReactFetch';

// TODO: Move this branching into the other module instead and just re-export.
const createElement: any = __DEV__
? createElementWithValidation
Expand Down
133 changes: 133 additions & 0 deletions packages/react/src/ReactFetch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import {
enableCache,
enableFetchInstrumentation,
} from 'shared/ReactFeatureFlags';

import ReactCurrentCache from './ReactCurrentCache';

function createFetchCache(): Map<string, Array<any>> {
return new Map();
}

const simpleCacheKey = '["GET",[],null,"follow",null,null,null,null]'; // generateCacheKey(new Request('https://blank'));

function generateCacheKey(request: Request): string {
// We pick the fields that goes into the key used to dedupe requests.
// We don't include the `cache` field, because we end up using whatever
// caching resulted from the first request.
// Notably we currently don't consider non-standard (or future) options.
// This might not be safe. TODO: warn for non-standard extensions differing.
// IF YOU CHANGE THIS UPDATE THE simpleCacheKey ABOVE.
return JSON.stringify([
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Oct 19, 2022

Choose a reason for hiding this comment

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

Since this isn't actually used as a key in a map we could just make it an explicit comparison of two Request objects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Intuitively that seems like it would be faster to me. But if you're not confident either way, seems fine to leave as is and benchmark later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OTOH this is less code

request.method,
Array.from(request.headers.entries()),
request.mode,
request.redirect,
request.credentials,
request.referrer,
request.referrerPolicy,
request.integrity,
]);
}

if (enableCache && enableFetchInstrumentation) {
if (typeof fetch === 'function') {
const originalFetch = fetch;
try {
// eslint-disable-next-line no-native-reassign
fetch = function fetch(
resource: URL | RequestInfo,
options?: RequestOptions,
) {
const dispatcher = ReactCurrentCache.current;
if (!dispatcher) {
// We're outside a cached scope.
return originalFetch(resource, options);
}
if (
options &&
options.signal &&
options.signal !== dispatcher.getCacheSignal()
) {
// If we're passed a signal that is not ours, then we assume that
// someone else controls the lifetime of this object and opts out of
// caching. It's effectively the opt-out mechanism.
// Ideally we should be able to check this on the Request but
// it always gets initialized with its own signal so we don't
// know if it's supposed to override - unless we also override the
// Request constructor.
return originalFetch(resource, options);
}
// Normalize the Request
let url: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need to normalize url if it's a relative path string? In case history.push gets called while the cache is alive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only for the fast path. Good catch. Originally I didn't have the fast path.

let cacheKey: string;
if (typeof resource === 'string' && !options) {
// Fast path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clever

cacheKey = simpleCacheKey;
url = resource;
} else {
// Normalize the request.
const request = new Request(resource, options);
if (
(request.method !== 'GET' && request.method !== 'HEAD') ||
// $FlowFixMe: keepalive is real
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆

request.keepalive
) {
// We currently don't dedupe requests that might have side-effects. Those
// have to be explicitly cached. We assume that the request doesn't have a
// body if it's GET or HEAD.
// keepalive gets treated the same as if you passed a custom cache signal.
return originalFetch(resource, options);
}
cacheKey = generateCacheKey(request);
url = request.url;
}
const cache = dispatcher.getCacheForType(createFetchCache);
const cacheEntries = cache.get(url);
let match;
if (cacheEntries === undefined) {
// We pass the original arguments here in case normalizing the Request
// doesn't include all the options in this environment.
match = originalFetch(resource, options);
cache.set(url, [cacheKey, match]);
} else {
// We use an array as the inner data structure since it's lighter and
Copy link
Collaborator

Choose a reason for hiding this comment

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

lighter than both {} and Map? I know those use arrays internally for small sizes right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't benchmark yet but maybe yea.

// we typically only expect to see one or two entries here.
for (let i = 0, l = cacheEntries.length; i < l; i += 2) {
const key = cacheEntries[i];
const value = cacheEntries[i + 1];
if (key === cacheKey) {
match = value;
// I would've preferred a labelled break but lint says no.
Copy link
Collaborator

Choose a reason for hiding this comment

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

change the lint config? labeled break is fine IMO

return match.then(response => response.clone());
}
}
match = originalFetch(resource, options);
cacheEntries.push(cacheKey, match);
}
// We clone the response so that each time you call this you get a new read
// of the body so that it can be read multiple times.
return match.then(response => response.clone());
};
// We don't expect to see any extra properties on fetch but if there are any,
// copy them over. Useful for extended fetch environments or mocks.
Object.assign(fetch, originalFetch);
} catch (error) {
// Log even in production just to make sure this is seen if only prod is frozen.
// eslint-disable-next-line react-internal/no-production-logging
console.warn(
'React was unable to patch the fetch() function in this environment. ' +
'Suspensey APIs might not work correctly as a result.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the first time we've used "Suspensey" in public facing messages? I think I dig it

);
}
}
}
144 changes: 144 additions & 0 deletions packages/react/src/__tests__/ReactFetch-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

// Polyfills for test environment
global.ReadableStream = require('web-streams-polyfill/ponyfill/es6').ReadableStream;
global.TextEncoder = require('util').TextEncoder;
global.TextDecoder = require('util').TextDecoder;
global.Headers = require('node-fetch').Headers;
global.Request = require('node-fetch').Request;
global.Response = require('node-fetch').Response;

let fetchCount = 0;
async function fetchMock(resource, options) {
fetchCount++;
const request = new Request(resource, options);
return new Response(
request.method +
' ' +
request.url +
' ' +
JSON.stringify(Array.from(request.headers.entries())),
);
}

let React;
let ReactServerDOMServer;
let ReactServerDOMClient;
let use;

describe('ReactFetch', () => {
beforeEach(() => {
jest.resetModules();
fetchCount = 0;
global.fetch = fetchMock;

React = require('react');
ReactServerDOMServer = require('react-server-dom-webpack/server.browser');
ReactServerDOMClient = require('react-server-dom-webpack/client');
use = React.experimental_use;
});

async function render(Component) {
const stream = ReactServerDOMServer.renderToReadableStream(<Component />);
return ReactServerDOMClient.createFromReadableStream(stream);
}

it('can fetch duplicates outside of render', async () => {
let response = await fetch('world');
let text = await response.text();
expect(text).toMatchInlineSnapshot(`"GET world []"`);
response = await fetch('world');
text = await response.text();
expect(text).toMatchInlineSnapshot(`"GET world []"`);
expect(fetchCount).toBe(2);
});

// @gate enableFetchInstrumentation && enableCache
it('can dedupe fetches inside of render', async () => {
function Component() {
const response = use(fetch('world'));
const text = use(response.text());
return text;
}
expect(await render(Component)).toMatchInlineSnapshot(`"GET world []"`);
expect(fetchCount).toBe(1);
});

// @gate enableFetchInstrumentation && enableCache
it('can dedupe fetches using Request and not', async () => {
function Component() {
const response = use(fetch('world'));
const text = use(response.text());
const sameRequest = new Request('world', {method: 'get'});
const response2 = use(fetch(sameRequest));
const text2 = use(response2.text());
return text + ' ' + text2;
}
expect(await render(Component)).toMatchInlineSnapshot(
`"GET world [] GET world []"`,
);
expect(fetchCount).toBe(1);
});

// @gate enableUseHook
it('can opt-out of deduping fetches inside of render with custom signal', async () => {
const controller = new AbortController();
function useCustomHook() {
return use(
fetch('world', {signal: controller.signal}).then(response =>
response.text(),
),
);
}
function Component() {
return useCustomHook() + ' ' + useCustomHook();
}
expect(await render(Component)).toMatchInlineSnapshot(
`"GET world [] GET world []"`,
);
expect(fetchCount).not.toBe(1);
});

// @gate enableUseHook
it('opts out of deduping for POST requests', async () => {
function useCustomHook() {
return use(
fetch('world', {method: 'POST'}).then(response => response.text()),
);
}
function Component() {
return useCustomHook() + ' ' + useCustomHook();
}
expect(await render(Component)).toMatchInlineSnapshot(
`"POST world [] POST world []"`,
);
expect(fetchCount).not.toBe(1);
});

// @gate enableFetchInstrumentation && enableCache
it('can dedupe fetches using same headers but not different', async () => {
function Component() {
const response = use(fetch('world', {headers: {a: 'A'}}));
const text = use(response.text());
const sameRequest = new Request('world', {
headers: new Headers({b: 'B'}),
});
const response2 = use(fetch(sameRequest));
const text2 = use(response2.text());
return text + ' ' + text2;
}
expect(await render(Component)).toMatchInlineSnapshot(
`"GET world [[\\"a\\",\\"A\\"]] GET world [[\\"b\\",\\"B\\"]]"`,
);
expect(fetchCount).toBe(2);
});
});
Loading