From d31c387ce6ee3782de632fdf1b7a1cff7952ca3f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 18 Oct 2022 16:55:37 -0400 Subject: [PATCH 1/3] Add fetch instrumentation in cached contexts --- package.json | 1 + packages/react-server/src/ReactFlightCache.js | 15 +- packages/react/src/React.js | 3 + packages/react/src/ReactFetch.js | 130 ++++++++++++++++ .../react/src/__tests__/ReactFetch-test.js | 144 ++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 1 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + yarn.lock | 5 + 15 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 packages/react/src/ReactFetch.js create mode 100644 packages/react/src/__tests__/ReactFetch-test.js diff --git a/package.json b/package.json index c3aa7eb0b9270..3c72f0d80a83e 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/packages/react-server/src/ReactFlightCache.js b/packages/react-server/src/ReactFlightCache.js index 239c59143bf83..54a34990e0114 100644 --- a/packages/react-server/src/ReactFlightCache.js +++ b/packages/react-server/src/ReactFlightCache.js @@ -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) { + 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 + currentCache.set(createSignal, entry); + } + return entry; }, getCacheForType(resourceType: () => T): T { if (!currentCache) { diff --git a/packages/react/src/React.js b/packages/react/src/React.js index 3c3cafe88bcf6..3cb0d13a13a05 100644 --- a/packages/react/src/React.js +++ b/packages/react/src/React.js @@ -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 diff --git a/packages/react/src/ReactFetch.js b/packages/react/src/ReactFetch.js new file mode 100644 index 0000000000000..a67c2c09a11e1 --- /dev/null +++ b/packages/react/src/ReactFetch.js @@ -0,0 +1,130 @@ +/** + * 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> { + 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([ + 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; + let cacheKey: string; + if (typeof resource === 'string' && !options) { + // Fast path. + 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 + 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 + // 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. + 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()); + }; + } 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.', + ); + } + } +} diff --git a/packages/react/src/__tests__/ReactFetch-test.js b/packages/react/src/__tests__/ReactFetch-test.js new file mode 100644 index 0000000000000..c68c697e29d6e --- /dev/null +++ b/packages/react/src/__tests__/ReactFetch-test.js @@ -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(); + 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); + }); +}); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 59122193ab10d..2cb806f64af9d 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -84,6 +84,7 @@ export const enableLegacyFBSupport = false; export const enableCache = __EXPERIMENTAL__; export const enableCacheElement = __EXPERIMENTAL__; +export const enableFetchInstrumentation = __EXPERIMENTAL__; export const enableTransitionTracing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 2c31bb577c707..e1dd754988834 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -29,6 +29,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = __PROFILE__; export const enableCache = false; export const enableCacheElement = true; +export const enableFetchInstrumentation = false; export const enableSchedulerDebugging = false; export const debugRenderPhaseSideEffectsForStrictMode = true; export const disableJavaScriptURLs = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 56fb6dd20b9fe..2b78003d289a7 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -22,6 +22,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = __PROFILE__; export const enableCache = false; export const enableCacheElement = false; +export const enableFetchInstrumentation = false; export const disableJavaScriptURLs = false; export const disableCommentsAsDOMContainers = true; export const disableInputAttributeSyncing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index b909a05fd79b2..bcbfca2fcc89e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -22,6 +22,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = false; export const enableCache = __EXPERIMENTAL__; export const enableCacheElement = __EXPERIMENTAL__; +export const enableFetchInstrumentation = __EXPERIMENTAL__; export const disableJavaScriptURLs = false; export const disableCommentsAsDOMContainers = true; export const disableInputAttributeSyncing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 309a9a5acdf8e..15670aab187b1 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -22,6 +22,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = false; export const enableCache = true; export const enableCacheElement = true; +export const enableFetchInstrumentation = false; export const disableJavaScriptURLs = false; export const disableCommentsAsDOMContainers = true; export const disableInputAttributeSyncing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 21c525917462a..d2b410f814c64 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -22,6 +22,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = false; export const enableCache = true; export const enableCacheElement = true; +export const enableFetchInstrumentation = false; export const enableSchedulerDebugging = false; export const disableJavaScriptURLs = false; export const disableCommentsAsDOMContainers = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 0252698dbad45..f6e927a020712 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -22,6 +22,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = false; export const enableCache = __EXPERIMENTAL__; export const enableCacheElement = __EXPERIMENTAL__; +export const enableFetchInstrumentation = __EXPERIMENTAL__; export const disableJavaScriptURLs = false; export const disableCommentsAsDOMContainers = true; export const disableInputAttributeSyncing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 4fdd8577871d1..83015bf39e9b7 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -22,6 +22,7 @@ export const enableProfilerNestedUpdateScheduledHook = false; export const enableUpdaterTracking = false; export const enableCache = true; export const enableCacheElement = true; +export const enableFetchInstrumentation = false; export const disableJavaScriptURLs = true; export const disableCommentsAsDOMContainers = true; export const disableInputAttributeSyncing = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 33f94ae63da40..a8ddcb63fa9fd 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -74,6 +74,7 @@ export const enableGetInspectorDataForInstanceInProduction = false; export const enableCache = true; export const enableCacheElement = true; +export const enableFetchInstrumentation = false; export const disableJavaScriptURLs = true; diff --git a/yarn.lock b/yarn.lock index 8c2ade1055d95..01849164caeaf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2,6 +2,11 @@ # yarn lockfile v1 +"@actuallyworks/node-fetch@^2.6.0": + version "2.6.0" + resolved "https://registry.yarnpkg.com/@actuallyworks/node-fetch/-/node-fetch-2.6.0.tgz#d1adc48813ab2866d86481950b99287c8d22d677" + integrity sha512-4G5iCA6JJk5TYy9GqSyyjGIOnf1w/K+XvCdVDm/Q6ryQGPhAwIGyZpIkPMLFStdjoswlgJTiY64j3yMw4S6l8g== + "@babel/cli@^7.10.5": version "7.10.5" resolved "https://registry.yarnpkg.com/@babel/cli/-/cli-7.10.5.tgz#57df2987c8cf89d0fc7d4b157ec59d7619f1b77a" From 3ea86d094a5f472c0fc74b4f96da9c641f46b4af Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 19 Oct 2022 16:01:07 -0400 Subject: [PATCH 2/3] Avoid unhandled rejection errors for Promises that we intentionally ignore In the final passes, we ignore the newly generated Promises and use the previous ones. This ensures that if those generate errors, that we intentionally ignore those. --- packages/react-reconciler/src/ReactFiberHooks.new.js | 7 +++++++ packages/react-reconciler/src/ReactFiberHooks.old.js | 7 +++++++ packages/react-server/src/ReactFizzHooks.js | 5 +++++ packages/react-server/src/ReactFlightHooks.js | 7 +++++++ 4 files changed, 26 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 244c0f77c6ed6..34fc709fdc388 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -768,6 +768,8 @@ if (enableUseMemoCacheHook) { }; } +function noop(): void {} + function use(usable: Usable): T { if (usable !== null && typeof usable === 'object') { // $FlowFixMe[method-unbinding] @@ -793,6 +795,11 @@ function use(usable: Usable): 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; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 283e9daedd046..938cddf02e1bd 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -768,6 +768,8 @@ if (enableUseMemoCacheHook) { }; } +function noop(): void {} + function use(usable: Usable): T { if (usable !== null && typeof usable === 'object') { // $FlowFixMe[method-unbinding] @@ -793,6 +795,11 @@ function use(usable: Usable): 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; diff --git a/packages/react-server/src/ReactFizzHooks.js b/packages/react-server/src/ReactFizzHooks.js index c8ceeddc400e4..0bd7c252c664b 100644 --- a/packages/react-server/src/ReactFizzHooks.js +++ b/packages/react-server/src/ReactFizzHooks.js @@ -610,6 +610,11 @@ function use(usable: Usable): 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; diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index fcbcbc0ff142e..1547f29dbcfbe 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -121,6 +121,8 @@ function useId(): string { return ':' + currentRequest.identifierPrefix + 'S' + id.toString(32) + ':'; } +function noop(): void {} + function use(usable: Usable): T { if (usable !== null && typeof usable === 'object') { // $FlowFixMe[method-unbinding] @@ -147,6 +149,11 @@ function use(usable: Usable): 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; From c94f72de0fc0598df9d3e6a7c10dcbbc37004fe0 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 19 Oct 2022 18:28:51 -0400 Subject: [PATCH 3/3] Add extra fetch properties if there were any --- packages/react/src/ReactFetch.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react/src/ReactFetch.js b/packages/react/src/ReactFetch.js index a67c2c09a11e1..7c39cefdf3f92 100644 --- a/packages/react/src/ReactFetch.js +++ b/packages/react/src/ReactFetch.js @@ -118,6 +118,9 @@ if (enableCache && enableFetchInstrumentation) { // 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