[react-cache] Remove `cache` as argument to `read` #13865

acdlite · 2018-10-17T00:42:57Z

Updated is API is Resource.read(key) instead of Resource.read(cache, key).

The cache is read from context using readContext.

This also removes cache invalidation entirely (other than the default LRU mechanism), as well as the ability to have multiple caches. We'll add it back once Context.write (some version of #13293) lands and we can implement it the right way.

Since there's now only a single cache (the global one), we don't actually need to use context yet, but I've added a dummy context anyway so the user gets an error if they attempt to read outside the render phase.

sizebot · 2018-10-17T00:49:52Z

	Fails
🚫	node` failed.

Log

Error:  { FetchError: invalid json response body at http://react.zpao.com/builds/master/_commits/80a0c05ce3e0e1babe5c8fc97ef24fd67cb4e735/results.json reason: Unexpected token < in JSON at position 0
    at /home/circleci/project/node_modules/node-fetch/lib/body.js:48:31
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)
  name: 'FetchError',
  message: 'invalid json response body at http://react.zpao.com/builds/master/_commits/80a0c05ce3e0e1babe5c8fc97ef24fd67cb4e735/results.json reason: Unexpected token < in JSON at position 0',
  type: 'invalid-json' }

Generated by 🚫 dangerJS

NE-SmallTown · 2018-10-17T02:22:54Z

packages/react-cache/src/__tests__/ReactCache-test.internal.js

+    ReactFeatureFlags = require('shared/ReactFeatureFlags');
+    ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
+    ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
+    ReactFeatureFlags.enableSuspense = true;


IIRC, enableSuspense has been removed in #13799 ?

gaearon · 2018-10-17T02:23:19Z

packages/react-cache/src/ReactCache.js

+    throw new Error(
+      'react-cache: read and preload may only be called from within a ' +
+        "component's render. They are not supported in event handlers or " +
+        'life-cycles.',


“lifecycle methods”?

gaearon · 2018-10-17T02:24:30Z

packages/react-cache/src/ReactCache.js

+
+function readContext(Context, observedBits) {
+  const dispatcher =
+    React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentOwner


Let’s hoist CurrentOwner? No need to access it many times.

NE-SmallTown · 2018-10-17T06:30:23Z

packages/react-cache/src/__tests__/ReactCache-test.internal.js

+      <Suspense>
+        <AsyncText ms={100} text={1} />
+        <AsyncText ms={100} text={2} />
+        <AsyncText ms={100} text={3} />


Should we use or add a PureComponent of AsyncText case like :

<AsyncText ms={100} text={2} /> <AsyncText ms={100} text={3} /> <AsyncText ms={100} text={1} />

here? IMO, we need a case which test the cache read and common reconciliation together.

This test does read from the cache? And idk what you mean by "common reconciliation"

Sorry. I mean, this just test the function component(the AsyncText) but in real world, we have class component which can be PureComponent or normal Component, so in the common reconciliation and update, they will go to different switch cases or codes, I think maybe it also should be tested with the cache read processing. @acdlite

sebmarkbage

Dan's comments.

aweary · 2018-10-23T17:00:17Z

packages/react-cache/src/ReactCache.js

-        resolvedRecord.status = Resolved;
-        resolvedRecord.suspender = null;
-        resolvedRecord.value = value;
+        if (newResult.status === Pending) {


What is this status check guarding against?

Guards against something resolving multiple times. Promises don't do that but a thenable might. Thanks for pointing this out, though, because now I realize I didn't write a test for it.

Updated is API is `Resource.read(key)` instead of `Resource.read(cache, key)`. The cache is read from context using `readContext`. This also removes cache invalidation entirely (other than the default LRU mechanism), as well as the ability to have multiple caches. We'll add it back once `Context.write` lands and we can implement it the right way. Since there's now only a single cache (the global one), we don't actually need to use context yet, but I've added a dummy context anyway so the user gets an error if they attempt to read outside the render phase.

Some breaking changes in the unstable APIs of Suspense: * No more need to use `enableSuspense` * flag in https://github.com/facebook/react/blob/master/packages/shared/ReactFeatureFlags.js#L48 * To generate custom build, use: `yarn build dom-client,core,react-cache,scheduler --type=NODE` * facebook/react@8af6728#diff-e7efbec821bc95957204c2fcbd251e74 * `<Placeholder delayMs />` renamed to `<Suspense maxDuration />` * facebook/react@8af6728 * facebook/react@d75c69e * react-cache * Add "unstable_" prefix - `createResource` becomes `unstable_createResource` * facebook/react@915e4ea * Remove `cache` as argument to `read` * facebook/react#13865

* [react-cache] Remove `cache` as argument to `read` Updated is API is `Resource.read(key)` instead of `Resource.read(cache, key)`. The cache is read from context using `readContext`. This also removes cache invalidation entirely (other than the default LRU mechanism), as well as the ability to have multiple caches. We'll add it back once `Context.write` lands and we can implement it the right way. Since there's now only a single cache (the global one), we don't actually need to use context yet, but I've added a dummy context anyway so the user gets an error if they attempt to read outside the render phase. * nits * Add test for thenables that resolve multiple times

acdlite requested review from bvaughn, sebmarkbage and gaearon October 17, 2018 00:42

facebook-github-bot added the CLA Signed label Oct 17, 2018

klamping mentioned this pull request Oct 17, 2018

React PR Test Report Listing klamping/react-webdriverio-tests#1

Open

acdlite force-pushed the react-cache branch 3 times, most recently from f4d7486 to 71f1677 Compare October 17, 2018 01:04

NE-SmallTown reviewed Oct 17, 2018

View reviewed changes

gaearon reviewed Oct 17, 2018

View reviewed changes

NE-SmallTown reviewed Oct 17, 2018

View reviewed changes

sebmarkbage requested changes Oct 18, 2018

View reviewed changes

acdlite force-pushed the react-cache branch from 4b2b017 to 42d5644 Compare October 19, 2018 02:34

acdlite requested a review from sebmarkbage October 19, 2018 02:34

acdlite force-pushed the react-cache branch from 42d5644 to 76307ac Compare October 19, 2018 02:40

acdlite requested review from sebmarkbage and removed request for sebmarkbage October 19, 2018 02:41

acdlite force-pushed the react-cache branch from 76307ac to e2e0700 Compare October 19, 2018 02:43

sebmarkbage approved these changes Oct 23, 2018

View reviewed changes

aweary reviewed Oct 23, 2018

View reviewed changes

acdlite mentioned this pull request Oct 23, 2018

Add "unstable_" prefix to react-cache and jest-react #13929

Merged

acdlite added 2 commits October 23, 2018 14:07

nits

f3a472f

acdlite force-pushed the react-cache branch from e2e0700 to f3a472f Compare October 23, 2018 21:16

Add test for thenables that resolve multiple times

61f1f7b

acdlite merged commit d8e03de into facebook:master Oct 23, 2018

gaearon mentioned this pull request Oct 23, 2018

Fix Suspense fixture #13932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[react-cache] Remove `cache` as argument to `read` #13865

[react-cache] Remove `cache` as argument to `read` #13865

acdlite commented Oct 17, 2018

sizebot commented Oct 17, 2018 •

edited

NE-SmallTown Oct 17, 2018 •

edited

gaearon Oct 17, 2018

gaearon Oct 17, 2018

NE-SmallTown Oct 17, 2018 •

edited

acdlite Oct 19, 2018

NE-SmallTown Oct 19, 2018 •

edited

sebmarkbage left a comment

aweary Oct 23, 2018 •

edited

acdlite Oct 23, 2018 •

edited

[react-cache] Remove cache as argument to read #13865

[react-cache] Remove cache as argument to read #13865

Conversation

acdlite commented Oct 17, 2018

sizebot commented Oct 17, 2018 • edited

Log

NE-SmallTown Oct 17, 2018 • edited

Choose a reason for hiding this comment

gaearon Oct 17, 2018

Choose a reason for hiding this comment

gaearon Oct 17, 2018

Choose a reason for hiding this comment

NE-SmallTown Oct 17, 2018 • edited

Choose a reason for hiding this comment

acdlite Oct 19, 2018

Choose a reason for hiding this comment

NE-SmallTown Oct 19, 2018 • edited

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

aweary Oct 23, 2018 • edited

Choose a reason for hiding this comment

acdlite Oct 23, 2018 • edited

Choose a reason for hiding this comment

[react-cache] Remove `cache` as argument to `read` #13865

[react-cache] Remove `cache` as argument to `read` #13865

sizebot commented Oct 17, 2018 •

edited

NE-SmallTown Oct 17, 2018 •

edited

NE-SmallTown Oct 17, 2018 •

edited

NE-SmallTown Oct 19, 2018 •

edited

aweary Oct 23, 2018 •

edited

acdlite Oct 23, 2018 •

edited