From ce37bfad5f61dcca7c2d0c0787bc65e64614c912 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 10 Aug 2020 20:45:12 +0100 Subject: [PATCH 01/12] Remove resolutions from test renderer package.json (#19577) --- packages/react-test-renderer/package.json | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/react-test-renderer/package.json b/packages/react-test-renderer/package.json index c2907a4e2981e..724eba635de01 100644 --- a/packages/react-test-renderer/package.json +++ b/packages/react-test-renderer/package.json @@ -27,9 +27,6 @@ "peerDependencies": { "react": "^17.0.0-alpha" }, - "resolutions": { - "react-shallow-renderer/react-is": "^17.0.0-alpha" - }, "files": [ "LICENSE", "README.md", From c8d9b8878a3a50fe94db5407727a3d5cba13f971 Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 11 Aug 2020 14:05:15 -0400 Subject: [PATCH 02/12] Speed up yarn cache in circle (#19566) --- .circleci/config.yml | 123 ++++++++++++++++++------------------------- 1 file changed, 52 insertions(+), 71 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 9039180938a40..559add55dbd64 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -7,17 +7,11 @@ aliases: - &environment TZ: /usr/share/zoneinfo/America/Los_Angeles - - &restore_yarn_cache + - &restore_node_modules restore_cache: name: Restore node_modules cache keys: - - v2-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }} - - v2-node-{{ arch }}-{{ .Branch }}- - - v2-node-{{ arch }}- - - &run_yarn - run: - name: Install Packages - command: yarn --frozen-lockfile + - v2-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }}-node-modules - &TEST_PARALLELISM 20 @@ -30,8 +24,7 @@ aliases: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: node ./scripts/rollup/consolidateBundleSizes.js - run: ./scripts/circleci/pack_and_store_artifact.sh - store_artifacts: @@ -59,13 +52,29 @@ jobs: - run: name: Nodejs Version command: node --version - - *restore_yarn_cache - - *run_yarn + - restore_cache: + name: Restore yarn cache + key: v2-node-{{ arch }}-{{ checksum "yarn.lock" }}-yarn + - run: + name: Install Packages + command: yarn --frozen-lockfile --cache-folder ~/.cache/yarn - save_cache: - name: Save node_modules cache - key: v2-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }} + # Store the yarn cache globally for all lock files with this same + # checksum. This will speed up the setup job for all PRs where the + # lockfile is the same. + name: Save yarn cache for future installs + key: v2-node-{{ arch }}-{{ checksum "yarn.lock" }}-yarn paths: - ~/.cache/yarn + - save_cache: + # Store node_modules for all jobs in this workflow so that they don't + # need to each run a yarn install for each job. This will speed up + # all jobs run on this branch with the same lockfile. + name: Save node_modules cache + # This cache key is per branch, a yarn install in setup is required. + key: v2-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }}-node-modules + paths: + - node_modules yarn_lint: docker: *docker @@ -73,8 +82,7 @@ jobs: steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: node ./scripts/prettier/index - run: node ./scripts/tasks/eslint - run: ./scripts/circleci/check_license.sh @@ -87,8 +95,7 @@ jobs: steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: node ./scripts/tasks/flow-ci RELEASE_CHANNEL_stable_yarn_test: @@ -98,8 +105,7 @@ jobs: steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=stable --ci yarn_test: @@ -108,8 +114,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --ci RELEASE_CHANNEL_stable_yarn_test_www: @@ -118,8 +123,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-classic --ci RELEASE_CHANNEL_stable_yarn_test_www_variant: @@ -128,8 +132,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-classic --variant --ci RELEASE_CHANNEL_stable_yarn_test_prod_www: @@ -138,8 +141,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-classic --prod --ci RELEASE_CHANNEL_stable_yarn_test_prod_www_variant: @@ -148,8 +150,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-classic --prod --variant --ci yarn_test_www: @@ -158,8 +159,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-modern --ci yarn_test_www_variant: @@ -168,8 +168,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-modern --variant --ci yarn_test_prod_www: @@ -178,8 +177,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-modern --prod --ci yarn_test_prod_www_variant: @@ -188,8 +186,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=www-modern --prod --variant --ci RELEASE_CHANNEL_stable_yarn_test_persistent: @@ -199,8 +196,7 @@ jobs: steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=stable --persistent --ci RELEASE_CHANNEL_stable_yarn_test_prod: @@ -210,8 +206,7 @@ jobs: steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=stable --prod --ci yarn_test_prod: @@ -220,8 +215,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=experimental --prod --ci RELEASE_CHANNEL_stable_yarn_build: @@ -230,8 +224,7 @@ jobs: parallelism: *TEST_PARALLELISM steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: environment: RELEASE_CHANNEL: stable @@ -257,8 +250,7 @@ jobs: parallelism: 20 steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: environment: RELEASE_CHANNEL: experimental @@ -285,8 +277,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: environment: RELEASE_CHANNEL: experimental @@ -305,8 +296,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules # This runs in the process_artifacts job, too, but it's faster to run # this step in both jobs instead of running the jobs sequentially - run: node ./scripts/rollup/consolidateBundleSizes.js @@ -321,8 +311,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules # This runs in the process_artifacts job, too, but it's faster to run # this step in both jobs instead of running the jobs sequentially - run: node ./scripts/rollup/consolidateBundleSizes.js @@ -337,8 +326,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn lint-build - run: scripts/circleci/check_minified_errors.sh @@ -348,8 +336,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: environment: RELEASE_CHANNEL: stable @@ -363,8 +350,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=stable --build --ci yarn_test_build: @@ -374,8 +360,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=experimental --build --ci yarn_test_build_devtools: @@ -384,8 +369,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --project=devtools --build --ci RELEASE_CHANNEL_stable_yarn_test_dom_fixtures: @@ -394,7 +378,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache + - *restore_node_modules - run: name: Run DOM fixture tests environment: @@ -410,8 +394,7 @@ jobs: environment: *environment steps: - checkout - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: name: Run fuzz tests command: | @@ -425,8 +408,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=stable --build --prod --ci yarn_test_build_prod: @@ -436,8 +418,7 @@ jobs: steps: - checkout - attach_workspace: *attach_workspace - - *restore_yarn_cache - - *run_yarn + - *restore_node_modules - run: yarn test --release-channel=experimental --build --prod --ci workflows: From b8ed6a1aa580e4de80f707293015d638d3252d63 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 Aug 2020 08:39:47 -0500 Subject: [PATCH 03/12] [Scheduler] Call postTask directly (#19551) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This updates the experimental Scheduler postTask build to call postTask directly, instead of managing our own custom queue and work loop. We still use a deadline 5ms mechanism to implement `shouldYield`. The main thing that postTask is currently missing is the continuation feature — when yielding to the main thread, the yielding task is sent to the back of the queue, instead of maintaining its position. While this would be nice to have, even without it, postTask may be good enough to replace our userspace implementation. We'll run some tests to see. --- .eslintrc.js | 6 + packages/scheduler/src/SchedulerPostTask.js | 249 ++++++++++++++++++ .../src/__tests__/SchedulerPostTask-test.js | 232 +++++++++------- .../forks/SchedulerHostConfig.post-task.js | 99 ------- packages/scheduler/unstable_post_task.js | 2 +- scripts/rollup/forks.js | 2 - scripts/rollup/validate/eslintrc.cjs.js | 2 + scripts/rollup/validate/eslintrc.cjs2015.js | 2 + scripts/rollup/validate/eslintrc.fb.js | 2 + scripts/rollup/validate/eslintrc.rn.js | 2 + scripts/rollup/validate/eslintrc.umd.js | 2 + 11 files changed, 400 insertions(+), 200 deletions(-) create mode 100644 packages/scheduler/src/SchedulerPostTask.js delete mode 100644 packages/scheduler/src/forks/SchedulerHostConfig.post-task.js diff --git a/.eslintrc.js b/.eslintrc.js index 179141ca82e1e..6998bca453f18 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -166,6 +166,12 @@ module.exports = { __webpack_require__: true, }, }, + { + files: ['packages/scheduler/**/*.js'], + globals: { + TaskController: true, + }, + }, ], globals: { diff --git a/packages/scheduler/src/SchedulerPostTask.js b/packages/scheduler/src/SchedulerPostTask.js new file mode 100644 index 0000000000000..b42f2114be2c3 --- /dev/null +++ b/packages/scheduler/src/SchedulerPostTask.js @@ -0,0 +1,249 @@ +/** + * 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 type {PriorityLevel} from './SchedulerPriorities'; + +declare class TaskController { + constructor(priority?: string): TaskController; + signal: mixed; + abort(): void; +} + +type PostTaskPriorityLevel = 'user-blocking' | 'user-visible' | 'background'; + +type CallbackNode = {| + _controller: TaskController, +|}; + +import { + ImmediatePriority, + UserBlockingPriority, + NormalPriority, + LowPriority, + IdlePriority, +} from './SchedulerPriorities'; + +export { + ImmediatePriority as unstable_ImmediatePriority, + UserBlockingPriority as unstable_UserBlockingPriority, + NormalPriority as unstable_NormalPriority, + IdlePriority as unstable_IdlePriority, + LowPriority as unstable_LowPriority, +}; + +// Capture local references to native APIs, in case a polyfill overrides them. +const perf = window.performance; + +// Use experimental Chrome Scheduler postTask API. +const scheduler = global.scheduler; + +const getCurrentTime = perf.now.bind(perf); + +export const unstable_now = getCurrentTime; + +// Scheduler periodically yields in case there is other work on the main +// thread, like user events. By default, it yields multiple times per frame. +// It does not attempt to align with frame boundaries, since most tasks don't +// need to be frame aligned; for those that do, use requestAnimationFrame. +const yieldInterval = 5; +let deadline = 0; + +let currentPriorityLevel_DEPRECATED = NormalPriority; + +// `isInputPending` is not available. Since we have no way of knowing if +// there's pending input, always yield at the end of the frame. +export function unstable_shouldYield() { + return getCurrentTime() >= deadline; +} + +export function unstable_requestPaint() { + // Since we yield every frame regardless, `requestPaint` has no effect. +} + +type SchedulerCallback = ( + didTimeout_DEPRECATED: boolean, +) => + | T + // May return a continuation + | SchedulerCallback; + +export function unstable_scheduleCallback( + priorityLevel: PriorityLevel, + callback: SchedulerCallback, + options?: {delay?: number}, +): CallbackNode { + let postTaskPriority; + switch (priorityLevel) { + case ImmediatePriority: + case UserBlockingPriority: + postTaskPriority = 'user-blocking'; + break; + case LowPriority: + case NormalPriority: + postTaskPriority = 'user-visible'; + break; + case IdlePriority: + postTaskPriority = 'background'; + break; + default: + postTaskPriority = 'user-visible'; + break; + } + + const controller = new TaskController(); + const postTaskOptions = { + priority: postTaskPriority, + delay: typeof options === 'object' && options !== null ? options.delay : 0, + signal: controller.signal, + }; + + const node = { + _controller: controller, + }; + + scheduler + .postTask( + runTask.bind(null, priorityLevel, postTaskPriority, node, callback), + postTaskOptions, + ) + .catch(handlePostTaskError); + + return node; +} + +function runTask( + priorityLevel: PriorityLevel, + postTaskPriority: PostTaskPriorityLevel, + node: CallbackNode, + callback: SchedulerCallback, +) { + deadline = getCurrentTime() + yieldInterval; + try { + currentPriorityLevel_DEPRECATED = priorityLevel; + const didTimeout_DEPRECATED = false; + const result = callback(didTimeout_DEPRECATED); + if (typeof result === 'function') { + // Assume this is a continuation + const continuation: SchedulerCallback = (result: any); + const continuationController = new TaskController(); + const continuationOptions = { + priority: postTaskPriority, + signal: continuationController.signal, + }; + // Update the original callback node's controller, since even though we're + // posting a new task, conceptually it's the same one. + node._controller = continuationController; + scheduler + .postTask( + runTask.bind( + null, + priorityLevel, + postTaskPriority, + node, + continuation, + ), + continuationOptions, + ) + .catch(handlePostTaskError); + } + } finally { + currentPriorityLevel_DEPRECATED = NormalPriority; + } +} + +function handlePostTaskError(error) { + // This error is either a user error thrown by a callback, or an AbortError + // as a result of a cancellation. + // + // User errors trigger a global `error` event even if we don't rethrow them. + // In fact, if we do rethrow them, they'll get reported to the console twice. + // I'm not entirely sure the current `postTask` spec makes sense here. If I + // catch a `postTask` call, it shouldn't trigger a global error. + // + // Abort errors are an implementation detail. We don't expose the + // TaskController to the user, nor do we expose the promise that is returned + // from `postTask`. So we shouldn't rethrow those, either, since there's no + // way to handle them. (If we did return the promise to the user, then it + // should be up to them to handle the AbortError.) + // + // In either case, we can suppress the error, barring changes to the spec + // or the Scheduler API. +} + +export function unstable_cancelCallback(node: CallbackNode) { + const controller = node._controller; + controller.abort(); +} + +export function unstable_runWithPriority( + priorityLevel: PriorityLevel, + callback: () => T, +): T { + const previousPriorityLevel = currentPriorityLevel_DEPRECATED; + currentPriorityLevel_DEPRECATED = priorityLevel; + try { + return callback(); + } finally { + currentPriorityLevel_DEPRECATED = previousPriorityLevel; + } +} + +export function unstable_getCurrentPriorityLevel() { + return currentPriorityLevel_DEPRECATED; +} + +export function unstable_next(callback: () => T): T { + let priorityLevel; + switch (currentPriorityLevel_DEPRECATED) { + case ImmediatePriority: + case UserBlockingPriority: + case NormalPriority: + // Shift down to normal priority + priorityLevel = NormalPriority; + break; + default: + // Anything lower than normal priority should remain at the current level. + priorityLevel = currentPriorityLevel_DEPRECATED; + break; + } + + const previousPriorityLevel = currentPriorityLevel_DEPRECATED; + currentPriorityLevel_DEPRECATED = priorityLevel; + try { + return callback(); + } finally { + currentPriorityLevel_DEPRECATED = previousPriorityLevel; + } +} + +export function unstable_wrapCallback(callback: () => T): () => T { + const parentPriorityLevel = currentPriorityLevel_DEPRECATED; + return () => { + const previousPriorityLevel = currentPriorityLevel_DEPRECATED; + currentPriorityLevel_DEPRECATED = parentPriorityLevel; + try { + return callback(); + } finally { + currentPriorityLevel_DEPRECATED = previousPriorityLevel; + } + }; +} + +export function unstable_forceFrameRate() {} + +export function unstable_pauseExecution() {} + +export function unstable_continueExecution() {} + +export function unstable_getFirstCallbackNode() { + return null; +} + +// Currently no profiling build +export const unstable_Profiling = null; diff --git a/packages/scheduler/src/__tests__/SchedulerPostTask-test.js b/packages/scheduler/src/__tests__/SchedulerPostTask-test.js index 32b91cd3ae0fc..8ad6228bb8a91 100644 --- a/packages/scheduler/src/__tests__/SchedulerPostTask-test.js +++ b/packages/scheduler/src/__tests__/SchedulerPostTask-test.js @@ -17,18 +17,16 @@ let runtime; let performance; let cancelCallback; let scheduleCallback; +let ImmediatePriority; let NormalPriority; +let UserBlockingPriority; +let LowPriority; +let IdlePriority; // The Scheduler postTask implementation uses a new postTask browser API to -// schedule work on the main thread. Most of our tests treat this as an -// implementation detail; however, the sequence and timing of browser -// APIs are not precisely specified, and can vary across browsers. -// -// To prevent regressions, we need the ability to simulate specific edge cases -// that we may encounter in various browsers. -// -// This test suite mocks all browser methods used in our implementation. It -// assumes as little as possible about the order and timing of events.s +// schedule work on the main thread. This test suite mocks all browser methods +// used in our implementation. It assumes as little as possible about the order +// and timing of events. describe('SchedulerPostTask', () => { beforeEach(() => { jest.resetModules(); @@ -37,18 +35,17 @@ describe('SchedulerPostTask', () => { jest.mock('scheduler', () => require.requireActual('scheduler/unstable_post_task'), ); - jest.mock('scheduler/src/SchedulerHostConfig', () => - require.requireActual( - 'scheduler/src/forks/SchedulerHostConfig.post-task.js', - ), - ); runtime = installMockBrowserRuntime(); performance = window.performance; Scheduler = require('scheduler'); cancelCallback = Scheduler.unstable_cancelCallback; scheduleCallback = Scheduler.unstable_scheduleCallback; + ImmediatePriority = Scheduler.unstable_ImmediatePriority; + UserBlockingPriority = Scheduler.unstable_UserBlockingPriority; NormalPriority = Scheduler.unstable_NormalPriority; + LowPriority = Scheduler.unstable_LowPriority; + IdlePriority = Scheduler.unstable_IdlePriority; }); afterEach(() => { @@ -58,14 +55,14 @@ describe('SchedulerPostTask', () => { }); function installMockBrowserRuntime() { - let hasPendingTask = false; - let timerIDCounter = 0; + let taskQueue = new Map(); let eventLog = []; // Mock window functions const window = {}; global.window = window; + let idCounter = 0; let currentTime = 0; window.performance = { now() { @@ -73,28 +70,33 @@ describe('SchedulerPostTask', () => { }, }; - window.setTimeout = (cb, delay) => { - const id = timerIDCounter++; - log(`Set Timer`); - // TODO - return id; - }; - window.clearTimeout = id => { - // TODO - }; - // Mock browser scheduler. const scheduler = {}; global.scheduler = scheduler; - let nextTask; - scheduler.postTask = function(callback) { - if (hasPendingTask) { - throw Error('Task already scheduled'); + scheduler.postTask = function(callback, {priority, signal}) { + const id = idCounter++; + log( + `Post Task ${id} [${priority === undefined ? '' : priority}]`, + ); + const controller = signal._controller; + return new Promise((resolve, reject) => { + taskQueue.set(controller, {id, callback, resolve, reject}); + }); + }; + + global.TaskController = class TaskController { + constructor() { + this.signal = {_controller: this}; + } + abort() { + const task = taskQueue.get(this); + if (task !== undefined) { + taskQueue.delete(this); + const reject = task.reject; + reject(new Error('Aborted')); + } } - log('Post Task'); - hasPendingTask = true; - nextTask = callback; }; function ensureLogIsEmpty() { @@ -105,22 +107,26 @@ describe('SchedulerPostTask', () => { function advanceTime(ms) { currentTime += ms; } - function fireNextTask() { + function flushTasks() { ensureLogIsEmpty(); - if (!hasPendingTask) { - throw Error('No task was scheduled'); - } - hasPendingTask = false; - - log('Task Event'); // If there's a continuation, it will call postTask again // which will set nextTask. That means we need to clear // nextTask before the invocation, otherwise we would // delete the continuation task. - const task = nextTask; - nextTask = null; - task(); + const prevTaskQueue = taskQueue; + taskQueue = new Map(); + for (const [, {id, callback, resolve, reject}] of prevTaskQueue) { + try { + log(`Task ${id} Fired`); + callback(false); + resolve(); + } catch (error) { + log(`Task ${id} errored [${error.message}]`); + reject(error); + continue; + } + } } function log(val) { eventLog.push(val); @@ -135,7 +141,7 @@ describe('SchedulerPostTask', () => { } return { advanceTime, - fireNextTask, + flushTasks, log, isLogEmpty, assertLog, @@ -144,16 +150,16 @@ describe('SchedulerPostTask', () => { it('task that finishes before deadline', () => { scheduleCallback(NormalPriority, () => { - runtime.log('Task'); + runtime.log('A'); }); - runtime.assertLog(['Post Task']); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'Task']); + runtime.assertLog(['Post Task 0 [user-visible]']); + runtime.flushTasks(); + runtime.assertLog(['Task 0 Fired', 'A']); }); it('task with continuation', () => { scheduleCallback(NormalPriority, () => { - runtime.log('Task'); + runtime.log('A'); while (!Scheduler.unstable_shouldYield()) { runtime.advanceTime(1); } @@ -162,13 +168,18 @@ describe('SchedulerPostTask', () => { runtime.log('Continuation'); }; }); - runtime.assertLog(['Post Task']); + runtime.assertLog(['Post Task 0 [user-visible]']); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'Task', 'Yield at 5ms', 'Post Task']); + runtime.flushTasks(); + runtime.assertLog([ + 'Task 0 Fired', + 'A', + 'Yield at 5ms', + 'Post Task 1 [user-visible]', + ]); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'Continuation']); + runtime.flushTasks(); + runtime.assertLog(['Task 1 Fired', 'Continuation']); }); it('multiple tasks', () => { @@ -178,55 +189,42 @@ describe('SchedulerPostTask', () => { scheduleCallback(NormalPriority, () => { runtime.log('B'); }); - runtime.assertLog(['Post Task']); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'A', 'B']); - }); - - it('multiple tasks with a yield in between', () => { - scheduleCallback(NormalPriority, () => { - runtime.log('A'); - runtime.advanceTime(4999); - }); - scheduleCallback(NormalPriority, () => { - runtime.log('B'); - }); - runtime.assertLog(['Post Task']); - runtime.fireNextTask(); runtime.assertLog([ - 'Task Event', - 'A', - // Ran out of time. Post a continuation event. - 'Post Task', + 'Post Task 0 [user-visible]', + 'Post Task 1 [user-visible]', ]); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'B']); + runtime.flushTasks(); + runtime.assertLog(['Task 0 Fired', 'A', 'Task 1 Fired', 'B']); }); it('cancels tasks', () => { const task = scheduleCallback(NormalPriority, () => { - runtime.log('Task'); + runtime.log('A'); }); - runtime.assertLog(['Post Task']); + runtime.assertLog(['Post Task 0 [user-visible]']); cancelCallback(task); + runtime.flushTasks(); runtime.assertLog([]); }); - it('throws when a task errors then continues in a new event', () => { + it('an error in one task does not affect execution of other tasks', () => { scheduleCallback(NormalPriority, () => { - runtime.log('Oops!'); throw Error('Oops!'); }); scheduleCallback(NormalPriority, () => { runtime.log('Yay'); }); - runtime.assertLog(['Post Task']); - - expect(() => runtime.fireNextTask()).toThrow('Oops!'); - runtime.assertLog(['Task Event', 'Oops!', 'Post Task']); - - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'Yay']); + runtime.assertLog([ + 'Post Task 0 [user-visible]', + 'Post Task 1 [user-visible]', + ]); + runtime.flushTasks(); + runtime.assertLog([ + 'Task 0 Fired', + 'Task 0 errored [Oops!]', + 'Task 1 Fired', + 'Yay', + ]); }); it('schedule new task after queue has emptied', () => { @@ -234,16 +232,16 @@ describe('SchedulerPostTask', () => { runtime.log('A'); }); - runtime.assertLog(['Post Task']); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'A']); + runtime.assertLog(['Post Task 0 [user-visible]']); + runtime.flushTasks(); + runtime.assertLog(['Task 0 Fired', 'A']); scheduleCallback(NormalPriority, () => { runtime.log('B'); }); - runtime.assertLog(['Post Task']); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'B']); + runtime.assertLog(['Post Task 1 [user-visible]']); + runtime.flushTasks(); + runtime.assertLog(['Task 1 Fired', 'B']); }); it('schedule new task after a cancellation', () => { @@ -251,17 +249,55 @@ describe('SchedulerPostTask', () => { runtime.log('A'); }); - runtime.assertLog(['Post Task']); + runtime.assertLog(['Post Task 0 [user-visible]']); cancelCallback(handle); - runtime.fireNextTask(); - runtime.assertLog(['Task Event']); + runtime.flushTasks(); + runtime.assertLog([]); scheduleCallback(NormalPriority, () => { runtime.log('B'); }); - runtime.assertLog(['Post Task']); - runtime.fireNextTask(); - runtime.assertLog(['Task Event', 'B']); + runtime.assertLog(['Post Task 1 [user-visible]']); + runtime.flushTasks(); + runtime.assertLog(['Task 1 Fired', 'B']); + }); + + it('schedules tasks at different priorities', () => { + scheduleCallback(ImmediatePriority, () => { + runtime.log('A'); + }); + scheduleCallback(UserBlockingPriority, () => { + runtime.log('B'); + }); + scheduleCallback(NormalPriority, () => { + runtime.log('C'); + }); + scheduleCallback(LowPriority, () => { + runtime.log('D'); + }); + scheduleCallback(IdlePriority, () => { + runtime.log('E'); + }); + runtime.assertLog([ + 'Post Task 0 [user-blocking]', + 'Post Task 1 [user-blocking]', + 'Post Task 2 [user-visible]', + 'Post Task 3 [user-visible]', + 'Post Task 4 [background]', + ]); + runtime.flushTasks(); + runtime.assertLog([ + 'Task 0 Fired', + 'A', + 'Task 1 Fired', + 'B', + 'Task 2 Fired', + 'C', + 'Task 3 Fired', + 'D', + 'Task 4 Fired', + 'E', + ]); }); }); diff --git a/packages/scheduler/src/forks/SchedulerHostConfig.post-task.js b/packages/scheduler/src/forks/SchedulerHostConfig.post-task.js deleted file mode 100644 index 1bd6bbea03ecf..0000000000000 --- a/packages/scheduler/src/forks/SchedulerHostConfig.post-task.js +++ /dev/null @@ -1,99 +0,0 @@ -/** - * 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. - */ - -// Capture local references to native APIs, in case a polyfill overrides them. -const perf = window.performance; -const setTimeout = window.setTimeout; -const clearTimeout = window.clearTimeout; - -function postTask(callback) { - // Use experimental Chrome Scheduler postTask API. - global.scheduler.postTask(callback); -} - -function getNow() { - return perf.now(); -} - -let isTaskLoopRunning = false; -let scheduledHostCallback = null; -let taskTimeoutID = -1; - -// Scheduler periodically yields in case there is other work on the main -// thread, like user events. By default, it yields multiple times per frame. -// It does not attempt to align with frame boundaries, since most tasks don't -// need to be frame aligned; for those that do, use requestAnimationFrame. -const yieldInterval = 5; -let deadline = 0; - -// `isInputPending` is not available. Since we have no way of knowing if -// there's pending input, always yield at the end of the frame. -export function shouldYieldToHost() { - return getNow() >= deadline; -} - -export function requestPaint() { - // Since we yield every frame regardless, `requestPaint` has no effect. -} - -export function forceFrameRate(fps) { - // No-op -} - -function performWorkUntilDeadline() { - if (scheduledHostCallback !== null) { - const currentTime = getNow(); - // Yield after `yieldInterval` ms, regardless of where we are in the vsync - // cycle. This means there's always time remaining at the beginning of - // the message event. - deadline = currentTime + yieldInterval; - const hasTimeRemaining = true; - try { - const hasMoreWork = scheduledHostCallback(hasTimeRemaining, currentTime); - if (!hasMoreWork) { - isTaskLoopRunning = false; - scheduledHostCallback = null; - } else { - // If there's more work, schedule the next message event at the end - // of the preceding one. - postTask(performWorkUntilDeadline); - } - } catch (error) { - // If a scheduler task throws, exit the current browser task so the - // error can be observed. - postTask(performWorkUntilDeadline); - throw error; - } - } else { - isTaskLoopRunning = false; - } -} - -export function requestHostCallback(callback) { - scheduledHostCallback = callback; - if (!isTaskLoopRunning) { - isTaskLoopRunning = true; - postTask(performWorkUntilDeadline); - } -} - -export function cancelHostCallback() { - scheduledHostCallback = null; -} - -export function requestHostTimeout(callback, ms) { - taskTimeoutID = setTimeout(() => { - callback(getNow()); - }, ms); -} - -export function cancelHostTimeout() { - clearTimeout(taskTimeoutID); - taskTimeoutID = -1; -} - -export const getCurrentTime = getNow; diff --git a/packages/scheduler/unstable_post_task.js b/packages/scheduler/unstable_post_task.js index aa14495a61abf..666eff8a85898 100644 --- a/packages/scheduler/unstable_post_task.js +++ b/packages/scheduler/unstable_post_task.js @@ -7,4 +7,4 @@ 'use strict'; -export * from './src/Scheduler'; +export * from './src/SchedulerPostTask'; diff --git a/scripts/rollup/forks.js b/scripts/rollup/forks.js index ecbbcd981e57f..e34daab32c6d3 100644 --- a/scripts/rollup/forks.js +++ b/scripts/rollup/forks.js @@ -209,8 +209,6 @@ const forks = Object.freeze({ entry === 'react-test-renderer' ) { return 'scheduler/src/forks/SchedulerHostConfig.mock'; - } else if (entry === 'scheduler/unstable_post_task') { - return 'scheduler/src/forks/SchedulerHostConfig.post-task'; } return 'scheduler/src/forks/SchedulerHostConfig.default'; }, diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js index 0e182bfefcdd7..4eae89839ae16 100644 --- a/scripts/rollup/validate/eslintrc.cjs.js +++ b/scripts/rollup/validate/eslintrc.cjs.js @@ -30,6 +30,8 @@ module.exports = { Int32Array: true, ArrayBuffer: true, + TaskController: true, + // Flight Uint8Array: true, Promise: true, diff --git a/scripts/rollup/validate/eslintrc.cjs2015.js b/scripts/rollup/validate/eslintrc.cjs2015.js index 5886ba8d91b41..81383a59643e9 100644 --- a/scripts/rollup/validate/eslintrc.cjs2015.js +++ b/scripts/rollup/validate/eslintrc.cjs2015.js @@ -30,6 +30,8 @@ module.exports = { Int32Array: true, ArrayBuffer: true, + TaskController: true, + // Flight Uint8Array: true, Promise: true, diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js index b05619d10bd0a..9c9f6b780a935 100644 --- a/scripts/rollup/validate/eslintrc.fb.js +++ b/scripts/rollup/validate/eslintrc.fb.js @@ -31,6 +31,8 @@ module.exports = { Int32Array: true, ArrayBuffer: true, + TaskController: true, + // Flight Uint8Array: true, Promise: true, diff --git a/scripts/rollup/validate/eslintrc.rn.js b/scripts/rollup/validate/eslintrc.rn.js index 49b993ed90d78..4fb8181ae7096 100644 --- a/scripts/rollup/validate/eslintrc.rn.js +++ b/scripts/rollup/validate/eslintrc.rn.js @@ -29,6 +29,8 @@ module.exports = { SharedArrayBuffer: true, Int32Array: true, ArrayBuffer: true, + + TaskController: true, }, parserOptions: { ecmaVersion: 5, diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index 8dfe4ec5630cf..e3786ffd3b6c2 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -34,6 +34,8 @@ module.exports = { Int32Array: true, ArrayBuffer: true, + TaskController: true, + // Flight Uint8Array: true, Promise: true, From b6e1d086043a801682ff01b00c7a623d529b46c0 Mon Sep 17 00:00:00 2001 From: Pascal Fong Kye Date: Wed, 12 Aug 2020 18:15:53 +0200 Subject: [PATCH 04/12] DevTools bug fix: Proxied methods should be safely dehydrated for display --- .../react-devtools-extensions/src/backend.js | 2 +- .../inspectedElementContext-test.js.snap | 1 + .../__tests__/inspectedElementContext-test.js | 16 ++++++++++++++++ packages/react-devtools-shared/src/hydration.js | 5 ++++- packages/react-devtools-shared/src/utils.js | 4 +++- 5 files changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/react-devtools-extensions/src/backend.js b/packages/react-devtools-extensions/src/backend.js index 357c286c2e287..aa8578a108646 100644 --- a/packages/react-devtools-extensions/src/backend.js +++ b/packages/react-devtools-extensions/src/backend.js @@ -72,7 +72,7 @@ function setup(hook) { initBackend(hook, agent, window); // Let the frontend know that the backend has attached listeners and is ready for messages. - // This covers the case of of syncing saved values after reloading/navigating while DevTools remain open. + // This covers the case of syncing saved values after reloading/navigating while DevTools remain open. bridge.send('extensionBackendInitialized'); // Setup React Native style editor if a renderer like react-native-web has injected it. diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap index 2ef217a451e61..1989aee6391a9 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap @@ -541,6 +541,7 @@ exports[`InspectedElementContext should support complex data types: 1: Inspected "object_of_objects": { "inner": {} }, + "proxy": {}, "react_element": {}, "regexp": {}, "set": { diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js index 4e292ecdbe5a7..29e8e379e4e43 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js @@ -549,6 +549,14 @@ describe('InspectedElementContext', () => { } const instance = new Class(); + const proxyInstance = new Proxy(() => {}, { + get: function(_, name) { + return function() { + return null; + }; + }, + }); + const container = document.createElement('div'); await utils.actAsync(() => ReactDOM.render( @@ -567,6 +575,7 @@ describe('InspectedElementContext', () => { map={mapShallow} map_of_maps={mapOfMaps} object_of_objects={objectOfObjects} + proxy={proxyInstance} react_element={} regexp={/abc/giu} set={setShallow} @@ -619,6 +628,7 @@ describe('InspectedElementContext', () => { map, map_of_maps, object_of_objects, + proxy, react_element, regexp, set, @@ -722,6 +732,12 @@ describe('InspectedElementContext', () => { ); expect(object_of_objects.inner[meta.preview_short]).toBe('{…}'); + expect(proxy[meta.inspectable]).toBe(false); + expect(proxy[meta.name]).toBe('function'); + expect(proxy[meta.type]).toBe('function'); + expect(proxy[meta.preview_long]).toBe('ƒ () {}'); + expect(proxy[meta.preview_short]).toBe('ƒ () {}'); + expect(react_element[meta.inspectable]).toBe(false); expect(react_element[meta.name]).toBe('span'); expect(react_element[meta.type]).toBe('react_element'); diff --git a/packages/react-devtools-shared/src/hydration.js b/packages/react-devtools-shared/src/hydration.js index f46048f75e646..23cb0a68fd9b2 100644 --- a/packages/react-devtools-shared/src/hydration.js +++ b/packages/react-devtools-shared/src/hydration.js @@ -151,7 +151,10 @@ export function dehydrate( inspectable: false, preview_short: formatDataForPreview(data, false), preview_long: formatDataForPreview(data, true), - name: data.name || 'function', + name: + typeof data.name === 'function' || !data.name + ? 'function' + : data.name, type, }; diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 3472e2c6dba9c..419aaac0c3dd0 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -539,7 +539,9 @@ export function formatDataForPreview( case 'html_element': return `<${truncateForDisplay(data.tagName.toLowerCase())} />`; case 'function': - return truncateForDisplay(`ƒ ${data.name}() {}`); + return truncateForDisplay( + `ƒ ${typeof data.name === 'function' ? '' : data.name}() {}`, + ); case 'string': return `"${data}"`; case 'bigint': From 629125555f381764c9f4943e877985274ebbc1b6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 Aug 2020 22:06:43 -0500 Subject: [PATCH 05/12] [Scheduler] Re-throw unhandled errors (#19595) Because `postTask` returns a promise, errors inside a `postTask` callback result in the promise being rejected. If we don't catch those errors, then the browser will report an "Unhandled promise rejection" error. This is a confusing message to see in the console, because the fact that `postTask` is a promise-based API is an implementation detail from the perspective of the developer. "Promise rejection" is a red herring. On the other hand, if we do catch those errors, then we need to report the error to the user in some other way. What we really want is the default error reporting behavior that a normal, non-Promise browser event gets. So, we'll re-throw inside `setTimeout`. --- packages/scheduler/src/SchedulerPostTask.js | 33 +++++++++---------- .../src/__tests__/SchedulerPostTask-test.js | 30 ++++++++--------- 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/packages/scheduler/src/SchedulerPostTask.js b/packages/scheduler/src/SchedulerPostTask.js index b42f2114be2c3..5d986d5127cb0 100644 --- a/packages/scheduler/src/SchedulerPostTask.js +++ b/packages/scheduler/src/SchedulerPostTask.js @@ -39,6 +39,7 @@ export { // Capture local references to native APIs, in case a polyfill overrides them. const perf = window.performance; +const setTimeout = window.setTimeout; // Use experimental Chrome Scheduler postTask API. const scheduler = global.scheduler; @@ -112,7 +113,7 @@ export function unstable_scheduleCallback( runTask.bind(null, priorityLevel, postTaskPriority, node, callback), postTaskOptions, ) - .catch(handlePostTaskError); + .catch(handleAbortError); return node; } @@ -150,30 +151,28 @@ function runTask( ), continuationOptions, ) - .catch(handlePostTaskError); + .catch(handleAbortError); } + } catch (error) { + // We're inside a `postTask` promise. If we don't handle this error, then it + // will trigger an "Unhandled promise rejection" error. We don't want that, + // but we do want the default error reporting behavior that normal + // (non-Promise) tasks get for unhandled errors. + // + // So we'll re-throw the error inside a regular browser task. + setTimeout(() => { + throw error; + }); } finally { currentPriorityLevel_DEPRECATED = NormalPriority; } } -function handlePostTaskError(error) { - // This error is either a user error thrown by a callback, or an AbortError - // as a result of a cancellation. - // - // User errors trigger a global `error` event even if we don't rethrow them. - // In fact, if we do rethrow them, they'll get reported to the console twice. - // I'm not entirely sure the current `postTask` spec makes sense here. If I - // catch a `postTask` call, it shouldn't trigger a global error. - // +function handleAbortError(error) { // Abort errors are an implementation detail. We don't expose the // TaskController to the user, nor do we expose the promise that is returned - // from `postTask`. So we shouldn't rethrow those, either, since there's no - // way to handle them. (If we did return the promise to the user, then it - // should be up to them to handle the AbortError.) - // - // In either case, we can suppress the error, barring changes to the spec - // or the Scheduler API. + // from `postTask`. So we should suppress them, since there's no way for the + // user to handle them. } export function unstable_cancelCallback(node: CallbackNode) { diff --git a/packages/scheduler/src/__tests__/SchedulerPostTask-test.js b/packages/scheduler/src/__tests__/SchedulerPostTask-test.js index 8ad6228bb8a91..19b6e330b82fb 100644 --- a/packages/scheduler/src/__tests__/SchedulerPostTask-test.js +++ b/packages/scheduler/src/__tests__/SchedulerPostTask-test.js @@ -70,6 +70,15 @@ describe('SchedulerPostTask', () => { }, }; + // Note: setTimeout is used to report errors and nothing else. + window.setTimeout = cb => { + try { + cb(); + } catch (error) { + runtime.log(`Error: ${error.message}`); + } + }; + // Mock browser scheduler. const scheduler = {}; global.scheduler = scheduler; @@ -116,16 +125,10 @@ describe('SchedulerPostTask', () => { // delete the continuation task. const prevTaskQueue = taskQueue; taskQueue = new Map(); - for (const [, {id, callback, resolve, reject}] of prevTaskQueue) { - try { - log(`Task ${id} Fired`); - callback(false); - resolve(); - } catch (error) { - log(`Task ${id} errored [${error.message}]`); - reject(error); - continue; - } + for (const [, {id, callback, resolve}] of prevTaskQueue) { + log(`Task ${id} Fired`); + callback(false); + resolve(); } } function log(val) { @@ -219,12 +222,7 @@ describe('SchedulerPostTask', () => { 'Post Task 1 [user-visible]', ]); runtime.flushTasks(); - runtime.assertLog([ - 'Task 0 Fired', - 'Task 0 errored [Oops!]', - 'Task 1 Fired', - 'Yay', - ]); + runtime.assertLog(['Task 0 Fired', 'Error: Oops!', 'Task 1 Fired', 'Yay']); }); it('schedule new task after queue has emptied', () => { From ccb6c39451b502c6b3ff3c014962827c54bae548 Mon Sep 17 00:00:00 2001 From: inottn Date: Thu, 13 Aug 2020 22:11:01 +0800 Subject: [PATCH 06/12] Remove unused argument (#19600) --- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 6 +++--- packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 9cd162cb01cf0..679a93bdd2790 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1470,7 +1470,7 @@ function handleError(root, thrownValue): void { } while (true); } -function pushDispatcher(root) { +function pushDispatcher() { const prevDispatcher = ReactCurrentDispatcher.current; ReactCurrentDispatcher.current = ContextOnlyDispatcher; if (prevDispatcher === null) { @@ -1586,7 +1586,7 @@ export function renderHasNotSuspendedYet(): boolean { function renderRootSync(root: FiberRoot, lanes: Lanes) { const prevExecutionContext = executionContext; executionContext |= RenderContext; - const prevDispatcher = pushDispatcher(root); + const prevDispatcher = pushDispatcher(); // If the root or lanes have changed, throw out the existing stack // and prepare a fresh one. Otherwise we'll continue where we left off. @@ -1661,7 +1661,7 @@ function workLoopSync() { function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { const prevExecutionContext = executionContext; executionContext |= RenderContext; - const prevDispatcher = pushDispatcher(root); + const prevDispatcher = pushDispatcher(); // If the root or lanes have changed, throw out the existing stack // and prepare a fresh one. Otherwise we'll continue where we left off. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 4cb6e5093377e..0aabd21aab470 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1454,7 +1454,7 @@ function handleError(root, thrownValue): void { } while (true); } -function pushDispatcher(root) { +function pushDispatcher() { const prevDispatcher = ReactCurrentDispatcher.current; ReactCurrentDispatcher.current = ContextOnlyDispatcher; if (prevDispatcher === null) { @@ -1570,7 +1570,7 @@ export function renderHasNotSuspendedYet(): boolean { function renderRootSync(root: FiberRoot, lanes: Lanes) { const prevExecutionContext = executionContext; executionContext |= RenderContext; - const prevDispatcher = pushDispatcher(root); + const prevDispatcher = pushDispatcher(); // If the root or lanes have changed, throw out the existing stack // and prepare a fresh one. Otherwise we'll continue where we left off. @@ -1645,7 +1645,7 @@ function workLoopSync() { function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { const prevExecutionContext = executionContext; executionContext |= RenderContext; - const prevDispatcher = pushDispatcher(root); + const prevDispatcher = pushDispatcher(); // If the root or lanes have changed, throw out the existing stack // and prepare a fresh one. Otherwise we'll continue where we left off. From c3ee973c5604078d5e9645a7e50db1842939d1e0 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 13 Aug 2020 11:16:35 -0400 Subject: [PATCH 07/12] Fix emoji character displayed in Chrome extension (#19603) --- packages/react-devtools-extensions/src/main.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-devtools-extensions/src/main.js b/packages/react-devtools-extensions/src/main.js index a405255ff59e8..00e3735c72c11 100644 --- a/packages/react-devtools-extensions/src/main.js +++ b/packages/react-devtools-extensions/src/main.js @@ -296,7 +296,7 @@ function createPanelIfReactLoaded() { let needsToSyncElementSelection = false; chrome.devtools.panels.create( - isChrome ? '⚛ Components' : 'Components', + isChrome ? '⚛️ Components' : 'Components', '', 'panel.html', extensionPanel => { @@ -326,7 +326,7 @@ function createPanelIfReactLoaded() { ); chrome.devtools.panels.create( - isChrome ? '⚛ Profiler' : 'Profiler', + isChrome ? '⚛️ Profiler' : 'Profiler', '', 'panel.html', extensionPanel => { From dab0854c5e66ca74ca0591fd312d6a654e5aaaf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 13 Aug 2020 12:17:33 -0400 Subject: [PATCH 08/12] Move commit passive unmount/mount to CommitWork (#19599) --- .../src/ReactFiberCommitWork.new.js | 163 +++++++++++++++++- .../src/ReactFiberWorkLoop.new.js | 161 ++--------------- 2 files changed, 172 insertions(+), 152 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6fda9c97cd1f0..79002ff6ae629 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -20,10 +20,14 @@ import type {FiberRoot} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {UpdateQueue} from './ReactUpdateQueue.new'; -import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; +import type { + Effect as HookEffect, + FunctionComponentUpdateQueue, +} from './ReactFiberHooks.new'; import type {Wakeable} from 'shared/ReactTypes'; import type {ReactPriorityLevel} from './ReactInternalTypes'; import type {OffscreenState} from './ReactFiberOffscreenComponent'; +import type {HookEffectTag} from './ReactHookEffectTags'; import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import { @@ -77,6 +81,8 @@ import { getCommitTime, recordLayoutEffectDuration, startLayoutEffectTimer, + recordPassiveEffectDuration, + startPassiveEffectTimer, } from './ReactProfilerTimer.new'; import {ProfileMode} from './ReactTypeOfMode'; import {commitUpdateQueue} from './ReactUpdateQueue.new'; @@ -121,6 +127,7 @@ import { NoEffect as NoHookEffect, HasEffect as HookHasEffect, Layout as HookLayout, + Passive as HookPassive, } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new'; import { @@ -308,7 +315,7 @@ function commitBeforeMutationLifeCycles( ); } -function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) { +function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -328,7 +335,43 @@ function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) { } } -function commitHookEffectListMount(tag: number, finishedWork: Fiber) { +// TODO: Remove this duplication. +function commitHookEffectListUnmount2( + // Tags to check for when deciding whether to unmount. e.g. to skip over + // layout effects + hookEffectTag: HookEffectTag, + fiber: Fiber, +): void { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + if ((tag & hookEffectTag) === hookEffectTag) { + const destroy = effect.destroy; + if (destroy !== undefined) { + effect.destroy = undefined; + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + safelyCallDestroy(fiber, destroy); + recordPassiveEffectDuration(fiber); + } else { + safelyCallDestroy(fiber, destroy); + } + } + } + effect = next; + } while (effect !== firstEffect); + } +} + +function commitHookEffectListMount(tag: HookEffectTag, finishedWork: Fiber) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -378,6 +421,83 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { } } +function invokePassiveEffectCreate(effect: HookEffect): void { + const create = effect.create; + effect.destroy = create(); +} + +// TODO: Remove this duplication. +function commitHookEffectListMount2(fiber: Fiber): void { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + + if ( + (tag & HookPassive) !== NoHookEffect && + (tag & HookHasEffect) !== NoHookEffect + ) { + if (__DEV__) { + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + invokeGuardedCallback( + null, + invokePassiveEffectCreate, + null, + effect, + ); + recordPassiveEffectDuration(fiber); + } else { + invokeGuardedCallback( + null, + invokePassiveEffectCreate, + null, + effect, + ); + } + if (hasCaughtError()) { + invariant(fiber !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + } else { + try { + const create = effect.create; + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + try { + startPassiveEffectTimer(); + effect.destroy = create(); + } finally { + recordPassiveEffectDuration(fiber); + } + } else { + effect.destroy = create(); + } + // TODO: This is missing the warning that exists in commitHookEffectListMount. + // The warning refers to useEffect but only applies to useLayoutEffect. + } catch (error) { + invariant(fiber !== null, 'Should be working on an effect.'); + captureCommitPhaseError(fiber, error); + } + } + } + + effect = next; + } while (effect !== firstEffect); + } +} + export function commitPassiveEffectDurations( finishedRoot: FiberRoot, finishedWork: Fiber, @@ -1709,13 +1829,45 @@ export function isSuspenseBoundaryBeingHidden( return false; } -function commitResetTextContent(current: Fiber) { +function commitResetTextContent(current: Fiber): void { if (!supportsMutation) { return; } resetTextContent(current.stateNode); } +function commitPassiveWork(finishedWork: Fiber): void { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { + commitHookEffectListUnmount2(HookPassive | HookHasEffect, finishedWork); + } + } +} + +function commitPassiveUnmount(current: Fiber): void { + switch (current.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: + commitHookEffectListUnmount2(HookPassive, current); + } +} + +function commitPassiveLifeCycles(finishedWork: Fiber): void { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Block: { + commitHookEffectListMount2(finishedWork); + } + } +} + export { commitBeforeMutationLifeCycles, commitResetTextContent, @@ -1725,4 +1877,7 @@ export { commitLifeCycles, commitAttachRef, commitDetachRef, + commitPassiveUnmount, + commitPassiveWork, + commitPassiveLifeCycles, }; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 679a93bdd2790..e35f87982daba 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -14,8 +14,6 @@ import type {ReactPriorityLevel} from './ReactInternalTypes'; import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; -import type {Effect as HookEffect} from './ReactFiberHooks.new'; -import type {HookEffectTag} from './ReactHookEffectTags'; import type {StackCursor} from './ReactFiberStack.new'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; @@ -53,7 +51,6 @@ import { } from './SchedulerWithReactIntegration.new'; import { NoEffect as NoHookEffect, - HasEffect as HookHasEffect, Passive as HookPassive, } from './ReactHookEffectTags'; import { @@ -206,12 +203,14 @@ import { commitPlacement, commitWork, commitDeletion, + commitPassiveUnmount, + commitPassiveWork, + commitPassiveLifeCycles as commitPassiveEffectOnFiber, commitDetachRef, commitAttachRef, commitPassiveEffectDurations, commitResetTextContent, isSuspenseBoundaryBeingHidden, - safelyCallDestroy, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -229,8 +228,6 @@ import { import { recordCommitTime, - recordPassiveEffectDuration, - startPassiveEffectTimer, startProfilerTimer, stopProfilerTimerIfRunningAndRecordDelta, } from './ReactProfilerTimer.new'; @@ -2738,11 +2735,6 @@ export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void { } } -function invokePassiveEffectCreate(effect: HookEffect): void { - const create = effect.create; - effect.destroy = create(); -} - function flushPassiveMountEffects(firstChild: Fiber): void { let fiber = firstChild; while (fiber !== null) { @@ -2753,93 +2745,15 @@ function flushPassiveMountEffects(firstChild: Fiber): void { } if ((fiber.effectTag & Update) !== NoEffect) { - switch (fiber.tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: - case Block: { - flushPassiveMountEffectsImpl(fiber); - } - } + setCurrentDebugFiberInDEV(fiber); + commitPassiveEffectOnFiber(fiber); + resetCurrentDebugFiberInDEV(); } fiber = fiber.sibling; } } -function flushPassiveMountEffectsImpl(fiber: Fiber): void { - setCurrentDebugFiberInDEV(fiber); - - const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); - const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - let effect = firstEffect; - do { - const {next, tag} = effect; - - if ( - (tag & HookPassive) !== NoHookEffect && - (tag & HookHasEffect) !== NoHookEffect - ) { - if (__DEV__) { - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - startPassiveEffectTimer(); - invokeGuardedCallback( - null, - invokePassiveEffectCreate, - null, - effect, - ); - recordPassiveEffectDuration(fiber); - } else { - invokeGuardedCallback( - null, - invokePassiveEffectCreate, - null, - effect, - ); - } - if (hasCaughtError()) { - invariant(fiber !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); - } - } else { - try { - const create = effect.create; - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - try { - startPassiveEffectTimer(); - effect.destroy = create(); - } finally { - recordPassiveEffectDuration(fiber); - } - } else { - effect.destroy = create(); - } - } catch (error) { - invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); - } - } - } - - effect = next; - } while (effect !== firstEffect); - - resetCurrentDebugFiberInDEV(); - } -} - function flushPassiveUnmountEffects(firstChild: Fiber): void { let fiber = firstChild; while (fiber !== null) { @@ -2866,16 +2780,11 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { } } - switch (fiber.tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: - case Block: { - const primaryEffectTag = fiber.effectTag & Passive; - if (primaryEffectTag !== NoEffect) { - flushPassiveUnmountEffectsImpl(fiber, HookPassive | HookHasEffect); - } - } + const primaryEffectTag = fiber.effectTag & Passive; + if (primaryEffectTag !== NoEffect) { + setCurrentDebugFiberInDEV(fiber); + commitPassiveWork(fiber); + resetCurrentDebugFiberInDEV(); } fiber = fiber.sibling; @@ -2898,52 +2807,8 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( } if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) { - switch (fiberToDelete.tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: - case Block: { - flushPassiveUnmountEffectsImpl(fiberToDelete, HookPassive); - } - } - } -} - -function flushPassiveUnmountEffectsImpl( - fiber: Fiber, - // Tags to check for when deciding whether to unmount. e.g. to skip over - // layout effects - hookEffectTag: HookEffectTag, -): void { - const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); - const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; - if (lastEffect !== null) { - setCurrentDebugFiberInDEV(fiber); - - const firstEffect = lastEffect.next; - let effect = firstEffect; - do { - const {next, tag} = effect; - if ((tag & hookEffectTag) === hookEffectTag) { - const destroy = effect.destroy; - if (destroy !== undefined) { - effect.destroy = undefined; - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - fiber.mode & ProfileMode - ) { - startPassiveEffectTimer(); - safelyCallDestroy(fiber, destroy); - recordPassiveEffectDuration(fiber); - } else { - safelyCallDestroy(fiber, destroy); - } - } - } - effect = next; - } while (effect !== firstEffect); - + setCurrentDebugFiberInDEV(fiberToDelete); + commitPassiveUnmount(fiberToDelete); resetCurrentDebugFiberInDEV(); } } From 1d5e10f7035f0d3bcbffcd057a15940b1a20b164 Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Thu, 13 Aug 2020 12:54:33 -0700 Subject: [PATCH 09/12] [eslint-plugin-react-hooks] Report constant constructions (#19590) * [eslint-plugin-react-cooks] Report constant constructions The dependency array passed to a React hook can be thought of as a list of cache keys. On each render, if any dependency is not `===` its previous value, the hook will be rerun. Constructing a new object/array/function/etc directly within your render function means that the value will be referentially unique on each render. If you then use that value as a hook dependency, that hook will get a "cache miss" on every render, making the dependency array useless. This can be especially dangerous since it can cascade. If a hook such as `useMemo` is rerun on each render, not only are we bypassing the option to avoid potentially expensive work, but the value _returned_ by `useMemo` may end up being referentially unique on each render causing other downstream hooks or memoized components to become deoptimized. * Fix/remove existing tests * Don't give an autofix of wrapping object declarations It may not be safe to just wrap the declaration of an object, since the object may get mutated. Only offer this autofix for functions which are unlikely to get mutated. Also, update the message to clarify that the entire construction of the value should get wrapped. * Handle the long tail of nodes that will be referentially unique * Catch let/var constant constructions on initial assignment * Trim trailing whitespace * Address feedback from @gaearon * Rename "assignment" to "initialization" * Add test for a constant construction used in multiple dependency arrays --- .../ESLintRuleExhaustiveDeps-test.js | 665 ++++++++++++++++-- .../src/ExhaustiveDeps.js | 224 ++++-- 2 files changed, 773 insertions(+), 116 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index de6d0aa4f7e07..26cda5e290875 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -56,7 +56,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [local]); @@ -94,9 +94,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); { - const local2 = {}; + const local2 = someFunc(); useCallback(() => { console.log(local1); console.log(local2); @@ -108,9 +108,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); function MyNestedComponent() { - const local2 = {}; + const local2 = someFunc(); useCallback(() => { console.log(local1); console.log(local2); @@ -122,7 +122,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); console.log(local); @@ -142,7 +142,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [,,,local,,,]); @@ -222,7 +222,7 @@ const tests = { { code: normalizeIndent` function MyComponent(props) { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(props.foo); console.log(props.bar); @@ -243,7 +243,7 @@ const tests = { console.log(props.bar); }, [props, props.foo]); - let color = {} + let color = someFunc(); useEffect(() => { console.log(props.foo.bar.baz); console.log(color); @@ -416,7 +416,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); function myEffect() { console.log(local); } @@ -731,7 +731,7 @@ const tests = { // direct assignments. code: normalizeIndent` function MyComponent(props) { - let obj = {}; + let obj = someFunc(); useEffect(() => { obj.foo = true; }, [obj]); @@ -1376,6 +1376,64 @@ const tests = { } `, }, + { + code: normalizeIndent` + function useFoo(foo){ + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + const foo = "hi!"; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + let {foo} = {foo: 1}; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo(){ + let [foo] = [1]; + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function useFoo() { + const foo = "fine"; + if (true) { + // Shadowed variable with constant construction in a nested scope is fine. + const foo = {}; + } + return useMemo(() => foo, [foo]); + } + `, + }, + { + code: normalizeIndent` + function MyComponent({foo}) { + return useMemo(() => foo, [foo]) + } + `, + }, + { + code: normalizeIndent` + function MyComponent() { + const foo = true ? "fine" : "also fine"; + return useMemo(() => foo, [foo]); + } + `, + }, ], invalid: [ { @@ -1494,7 +1552,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, []); @@ -1510,7 +1568,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [local]); @@ -1636,7 +1694,7 @@ const tests = { // Regression test code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { if (true) { console.log(local); @@ -1654,7 +1712,7 @@ const tests = { desc: 'Update the dependencies array to be: [local]', output: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { if (true) { console.log(local); @@ -1742,9 +1800,9 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); { - const local2 = {}; + const local2 = someFunc(); useEffect(() => { console.log(local1); console.log(local2); @@ -1762,9 +1820,9 @@ const tests = { desc: 'Update the dependencies array to be: [local1, local2]', output: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); { - const local2 = {}; + const local2 = someFunc(); useEffect(() => { console.log(local1); console.log(local2); @@ -1846,7 +1904,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -1868,7 +1926,7 @@ const tests = { desc: 'Update the dependencies array to be: [local2]', output: normalizeIndent` function MyComponent() { - const local1 = {}; + const local1 = someFunc(); function MyNestedComponent() { const local2 = {}; useCallback(() => { @@ -2295,7 +2353,7 @@ const tests = { { code: normalizeIndent` function MyComponent() { - const local = {}; + const local = someFunc(); useEffect(() => { console.log(local); }, [local, ...dependencies]); @@ -5311,7 +5369,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `Move it inside the useEffect callback. Alternatively, ` + - `wrap the 'handleNext' definition into its own useCallback() Hook.`, + `wrap the definition of 'handleNext' in its own useCallback() Hook.`, // Not gonna fix a function definition // because it's not always safe due to hoisting. suggestions: undefined, @@ -5340,7 +5398,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `Move it inside the useEffect callback. Alternatively, ` + - `wrap the 'handleNext' definition into its own useCallback() Hook.`, + `wrap the definition of 'handleNext' in its own useCallback() Hook.`, // We don't fix moving (too invasive). But that's the suggested fix // when only effect uses this function. Otherwise, we'd useCallback. suggestions: undefined, @@ -5373,13 +5431,13 @@ const tests = { message: `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + - `To fix this, wrap the 'handleNext' definition into its own useCallback() Hook.`, + `To fix this, wrap the definition of 'handleNext' in its own useCallback() Hook.`, // We fix this one with useCallback since it's // the easy fix and you can't just move it into effect. suggestions: [ { desc: - "Wrap the 'handleNext' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { let [, setState] = useState(); @@ -5428,21 +5486,21 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 14) change on every render. Move it inside the useEffect callback. ' + - "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 17) change on every render. Move it inside the useLayoutEffect callback. ' + - "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 20) change on every render. Move it inside the useMemo callback. ' + - "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5480,21 +5538,21 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 15) change on every render. Move it inside the useEffect callback. ' + - "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 19) change on every render. Move it inside the useLayoutEffect callback. ' + - "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 23) change on every render. Move it inside the useMemo callback. ' + - "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.", + "Alternatively, wrap the definition of 'handleNext3' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5541,20 +5599,20 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 15) change on every render. To fix this, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + '(at line 19) change on every render. To fix this, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", // Suggestion wraps into useCallback where possible (variables only) // because they are only referenced outside the effect. suggestions: [ { desc: - "Wrap the 'handleNext2' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext2' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5598,13 +5656,13 @@ const tests = { message: "The 'handleNext3' function makes the dependencies of useMemo Hook " + '(at line 23) change on every render. To fix this, wrap the ' + - "'handleNext3' definition into its own useCallback() Hook.", + "definition of 'handleNext3' in its own useCallback() Hook.", // Autofix wraps into useCallback where possible (variables only) // because they are only referenced outside the effect. suggestions: [ { desc: - "Wrap the 'handleNext3' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext3' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { function handleNext1() { @@ -5675,11 +5733,11 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 12) change on every render. To fix this, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5705,11 +5763,11 @@ const tests = { message: "The 'handleNext1' function makes the dependencies of useEffect Hook " + '(at line 16) change on every render. To fix this, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + "definition of 'handleNext1' in its own useCallback() Hook.", suggestions: [ { desc: - "Wrap the 'handleNext1' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext1' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { const handleNext1 = useCallback(() => { @@ -5735,14 +5793,14 @@ const tests = { message: "The 'handleNext2' function makes the dependencies of useEffect Hook " + '(at line 12) change on every render. To fix this, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, { message: "The 'handleNext2' function makes the dependencies of useEffect Hook " + '(at line 16) change on every render. To fix this, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + "definition of 'handleNext2' in its own useCallback() Hook.", suggestions: undefined, }, ], @@ -5767,8 +5825,8 @@ const tests = { { message: "The 'handleNext' function makes the dependencies of useEffect Hook " + - '(at line 13) change on every render. To fix this, wrap the ' + - "'handleNext' definition into its own useCallback() Hook.", + '(at line 13) change on every render. To fix this, wrap the definition of ' + + "'handleNext' in its own useCallback() Hook.", // Normally we'd suggest moving handleNext inside an // effect. But it's used more than once. // TODO: our autofix here isn't quite sufficient because @@ -5776,7 +5834,7 @@ const tests = { suggestions: [ { desc: - "Wrap the 'handleNext' definition into its own useCallback() Hook.", + "Wrap the definition of 'handleNext' in its own useCallback() Hook.", output: normalizeIndent` function MyComponent(props) { let handleNext = useCallback(() => { @@ -5820,7 +5878,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 14) change on every render. ` + `Move it inside the useEffect callback. Alternatively, wrap the ` + - `'handleNext' definition into its own useCallback() Hook.`, + `definition of 'handleNext' in its own useCallback() Hook.`, suggestions: undefined, }, ], @@ -6085,7 +6143,7 @@ const tests = { message: `The 'increment' function makes the dependencies of useEffect Hook ` + `(at line 14) change on every render. Move it inside the useEffect callback. ` + - `Alternatively, wrap the \'increment\' definition into its own ` + + `Alternatively, wrap the definition of \'increment\' in its own ` + `useCallback() Hook.`, suggestions: undefined, }, @@ -6967,6 +7025,499 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = []; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' array makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = () => {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the definition of 'foo' in its own " + + 'useCallback() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = function bar(){}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' function makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the definition of 'foo' in its own " + + 'useCallback() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = class {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' class makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = true ? {} : "fine"; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar || {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar ?? {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar && {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = bar ? baz ? {} : null : null; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' conditional could make the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + let foo = {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + var foo = {}; + useMemo(() => foo, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 4) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useCallback(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useCallback Hook (at line 6) change on every render. " + + "Move it inside the useCallback callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useEffect Hook (at line 6) change on every render. " + + "Move it inside the useEffect callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useLayoutEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + + "Move it inside the useLayoutEffect callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Component() { + const foo = {}; + useImperativeHandle( + ref, + () => { + console.log(foo); + }, + [foo] + ); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useImperativeHandle Hook (at line 9) change on every render. " + + "Move it inside the useImperativeHandle callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo(section) { + const foo = section.section_components?.edges ?? []; + useEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' logical expression could make the dependencies of useEffect Hook (at line 6) change on every render. " + + "Move it inside the useEffect callback. Alternatively, wrap the initialization of 'foo' in its own " + + 'useMemo() Hook.', + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo(section) { + const foo = {}; + console.log(foo); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 7) change on every render. " + + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = <>Hi!; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' JSX fragment makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo =
Hi!
; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' JSX element makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = bar = {}; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' assignment expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = new String('foo'); // Note 'foo' will be boxed, and thus an object and thus compared by reference. + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = new Map([]); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object construction makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = /reg/; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' regular expression makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = ({}: any); + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + class Bar {}; + useMemo(() => { + console.log(new Bar()); + }, [Bar]); + } + `, + errors: [ + { + message: + "The 'Bar' class makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'Bar' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, + { + code: normalizeIndent` + function Foo() { + const foo = {}; + useLayoutEffect(() => { + console.log(foo); + }, [foo]); + useEffect(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useLayoutEffect Hook (at line 6) change on every render. " + + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + { + message: + "The 'foo' object makes the dependencies of useEffect Hook (at line 9) change on every render. " + + "To fix this, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, ], }; @@ -7345,6 +7896,24 @@ const testsTypescript = { }, ], }, + { + code: normalizeIndent` + function Foo() { + const foo = {} as any; + useMemo(() => { + console.log(foo); + }, [foo]); + } + `, + errors: [ + { + message: + "The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " + + "Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.", + suggestions: undefined, + }, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 9e344e8662d6d..ab552c305a4db 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -844,59 +844,80 @@ export default { unnecessaryDependencies.size; if (problemCount === 0) { - // If nothing else to report, check if some callbacks - // are bare and would invalidate on every render. - const bareFunctions = scanForDeclaredBareFunctions({ + // If nothing else to report, check if some dependencies would + // invalidate on every render. + const constructions = scanForConstructions({ declaredDependencies, declaredDependenciesNode, componentScope, scope, }); - bareFunctions.forEach(({fn, suggestUseCallback}) => { - let message = - `The '${fn.name.name}' function makes the dependencies of ` + - `${reactiveHookName} Hook (at line ${declaredDependenciesNode.loc.start.line}) ` + - `change on every render.`; - if (suggestUseCallback) { - message += - ` To fix this, ` + - `wrap the '${fn.name.name}' definition into its own useCallback() Hook.`; - } else { - message += - ` Move it inside the ${reactiveHookName} callback. ` + - `Alternatively, wrap the '${fn.name.name}' definition into its own useCallback() Hook.`; - } + constructions.forEach( + ({construction, isUsedOutsideOfHook, depType}) => { + const wrapperHook = + depType === 'function' ? 'useCallback' : 'useMemo'; - let suggest; - // Only handle the simple case: arrow functions. - // Wrapping function declarations can mess up hoisting. - if (suggestUseCallback && fn.type === 'Variable') { - suggest = [ - { - desc: `Wrap the '${fn.name.name}' definition into its own useCallback() Hook.`, - fix(fixer) { - return [ - // TODO: also add an import? - fixer.insertTextBefore(fn.node.init, 'useCallback('), - // TODO: ideally we'd gather deps here but it would require - // restructuring the rule code. This will cause a new lint - // error to appear immediately for useCallback. Note we're - // not adding [] because would that changes semantics. - fixer.insertTextAfter(fn.node.init, ')'), - ]; + const constructionType = + depType === 'function' ? 'definition' : 'initialization'; + + const defaultAdvice = `wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`; + + const advice = isUsedOutsideOfHook + ? `To fix this, ${defaultAdvice}` + : `Move it inside the ${reactiveHookName} callback. Alternatively, ${defaultAdvice}`; + + const causation = + depType === 'conditional' || depType === 'logical expression' + ? 'could make' + : 'makes'; + + const message = + `The '${construction.name.name}' ${depType} ${causation} the dependencies of ` + + `${reactiveHookName} Hook (at line ${declaredDependenciesNode.loc.start.line}) ` + + `change on every render. ${advice}`; + + let suggest; + // Only handle the simple case of variable assignments. + // Wrapping function declarations can mess up hoisting. + if ( + isUsedOutsideOfHook && + construction.type === 'Variable' && + // Objects may be mutated ater construction, which would make this + // fix unsafe. Functions _probably_ won't be mutated, so we'll + // allow this fix for them. + depType === 'function' + ) { + suggest = [ + { + desc: `Wrap the ${constructionType} of '${construction.name.name}' in its own ${wrapperHook}() Hook.`, + fix(fixer) { + const [before, after] = + wrapperHook === 'useMemo' + ? [`useMemo(() => { return `, '; })'] + : ['useCallback(', ')']; + return [ + // TODO: also add an import? + fixer.insertTextBefore(construction.node.init, before), + // TODO: ideally we'd gather deps here but it would require + // restructuring the rule code. This will cause a new lint + // error to appear immediately for useCallback. Note we're + // not adding [] because would that changes semantics. + fixer.insertTextAfter(construction.node.init, after), + ]; + }, }, - }, - ]; - } - // TODO: What if the function needs to change on every render anyway? - // Should we suggest removing effect deps as an appropriate fix too? - reportProblem({ - // TODO: Why not report this at the dependency site? - node: fn.node, - message, - suggest, - }); - }); + ]; + } + // TODO: What if the function needs to change on every render anyway? + // Should we suggest removing effect deps as an appropriate fix too? + reportProblem({ + // TODO: Why not report this at the dependency site? + node: construction.node, + message, + suggest, + }); + }, + ); return; } @@ -1381,50 +1402,116 @@ function collectRecommendations({ }; } -// Finds functions declared as dependencies +// If the node will result in constructing a referentially unique value, return +// its human readable type name, else return null. +function getConstructionExpressionType(node) { + switch (node.type) { + case 'ObjectExpression': + return 'object'; + case 'ArrayExpression': + return 'array'; + case 'ArrowFunctionExpression': + case 'FunctionExpression': + return 'function'; + case 'ClassExpression': + return 'class'; + case 'ConditionalExpression': + if ( + getConstructionExpressionType(node.consequent) != null || + getConstructionExpressionType(node.alternate) != null + ) { + return 'conditional'; + } + return null; + case 'LogicalExpression': + if ( + getConstructionExpressionType(node.left) != null || + getConstructionExpressionType(node.right) != null + ) { + return 'logical expression'; + } + return null; + case 'JSXFragment': + return 'JSX fragment'; + case 'JSXElement': + return 'JSX element'; + case 'AssignmentExpression': + if (getConstructionExpressionType(node.right) != null) { + return 'assignment expression'; + } + return null; + case 'NewExpression': + return 'object construction'; + case 'Literal': + if (node.value instanceof RegExp) { + return 'regular expression'; + } + return null; + case 'TypeCastExpression': + return getConstructionExpressionType(node.expression); + case 'TSAsExpression': + return getConstructionExpressionType(node.expression); + } + return null; +} + +// Finds variables declared as dependencies // that would invalidate on every render. -function scanForDeclaredBareFunctions({ +function scanForConstructions({ declaredDependencies, declaredDependenciesNode, componentScope, scope, }) { - const bareFunctions = declaredDependencies + const constructions = declaredDependencies .map(({key}) => { - const fnRef = componentScope.set.get(key); - if (fnRef == null) { + const ref = componentScope.variables.find(v => v.name === key); + if (ref == null) { return null; } - const fnNode = fnRef.defs[0]; - if (fnNode == null) { + + const node = ref.defs[0]; + if (node == null) { return null; } // const handleChange = function () {} // const handleChange = () => {} + // const foo = {} + // const foo = [] + // etc. if ( - fnNode.type === 'Variable' && - fnNode.node.type === 'VariableDeclarator' && - fnNode.node.init != null && - (fnNode.node.init.type === 'ArrowFunctionExpression' || - fnNode.node.init.type === 'FunctionExpression') + node.type === 'Variable' && + node.node.type === 'VariableDeclarator' && + node.node.id.type === 'Identifier' && // Ensure this is not destructed assignment + node.node.init != null ) { - return fnRef; + const constantExpressionType = getConstructionExpressionType( + node.node.init, + ); + if (constantExpressionType != null) { + return [ref, constantExpressionType]; + } } // function handleChange() {} if ( - fnNode.type === 'FunctionName' && - fnNode.node.type === 'FunctionDeclaration' + node.type === 'FunctionName' && + node.node.type === 'FunctionDeclaration' ) { - return fnRef; + return [ref, 'function']; + } + + // class Foo {} + if (node.type === 'ClassName' && node.node.type === 'ClassDeclaration') { + return [ref, 'class']; } return null; }) .filter(Boolean); - function isUsedOutsideOfHook(fnRef) { + function isUsedOutsideOfHook(ref) { let foundWriteExpr = false; - for (let i = 0; i < fnRef.references.length; i++) { - const reference = fnRef.references[i]; + for (let i = 0; i < ref.references.length; i++) { + const reference = ref.references[i]; if (reference.writeExpr) { if (foundWriteExpr) { // Two writes to the same function. @@ -1450,9 +1537,10 @@ function scanForDeclaredBareFunctions({ return false; } - return bareFunctions.map(fnRef => ({ - fn: fnRef.defs[0], - suggestUseCallback: isUsedOutsideOfHook(fnRef), + return constructions.map(([ref, depType]) => ({ + construction: ref.defs[0], + depType, + isUsedOutsideOfHook: isUsedOutsideOfHook(ref), })); } From fe6d05229f29c59ea2bbbd616cdfa0c6e2ea03fc Mon Sep 17 00:00:00 2001 From: Clay Tercek Date: Fri, 14 Aug 2020 08:05:53 -0400 Subject: [PATCH 10/12] fix event.relatedTarget fallback logic for firefox (#19607) * fix event.relatedTarget fallback logic for firefox * check if relatedTarget is undefined for fallback --- packages/react-dom/src/events/SyntheticEvent.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/events/SyntheticEvent.js b/packages/react-dom/src/events/SyntheticEvent.js index a6c7e60411198..e226e95833cae 100644 --- a/packages/react-dom/src/events/SyntheticEvent.js +++ b/packages/react-dom/src/events/SyntheticEvent.js @@ -170,12 +170,12 @@ export const MouseEventInterface = { button: 0, buttons: 0, relatedTarget: function(event) { - return ( - event.relatedTarget || - (event.fromElement === event.srcElement + if (event.relatedTarget === undefined) + return event.fromElement === event.srcElement ? event.toElement - : event.fromElement) - ); + : event.fromElement; + + return event.relatedTarget; }, movementX: function(event) { if ('movementX' in event) { From 9abc2785cb070148d64fae81e523246b90b92016 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 13 Aug 2020 11:49:24 -0500 Subject: [PATCH 11/12] Remove wasteful checks from `shouldYield` `shouldYield` will currently return `true` if there's a higher priority task in the Scheduler queue. Since we yield every 5ms anyway, this doesn't really have any practical benefit. On the contrary, the extra checks on every `shouldYield` call are wasteful. --- .../__tests__/ReactProfiler-test.internal.js | 10 +++------- packages/scheduler/src/Scheduler.js | 17 +---------------- .../scheduler/src/__tests__/Scheduler-test.js | 13 ++++++------- 3 files changed, 10 insertions(+), 30 deletions(-) diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 78672895584fd..c3d1dbb3cea71 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -2481,15 +2481,11 @@ describe('Profiler', () => { // Errors that happen inside of a subscriber should throw, throwInOnWorkStarted = true; expect(Scheduler).toFlushAndThrow('Expected error onWorkStarted'); - // Rendering was interrupted by the error that was thrown - expect(Scheduler).toHaveYielded([]); - // Rendering continues in the next task - expect(Scheduler).toFlushAndYield(['Component:text']); throwInOnWorkStarted = false; + // Rendering was interrupted by the error that was thrown, then + // continued and finished in the next task. + expect(Scheduler).toHaveYielded(['Component:text']); expect(onWorkStarted).toHaveBeenCalled(); - - // But the React work should have still been processed. - expect(Scheduler).toFlushAndYield([]); const tree = renderer.toTree(); expect(tree.type).toBe(Component); expect(tree.props.children).toBe('text'); diff --git a/packages/scheduler/src/Scheduler.js b/packages/scheduler/src/Scheduler.js index ff9e06c7a345d..9162da0a0875c 100644 --- a/packages/scheduler/src/Scheduler.js +++ b/packages/scheduler/src/Scheduler.js @@ -393,21 +393,6 @@ function unstable_getCurrentPriorityLevel() { return currentPriorityLevel; } -function unstable_shouldYield() { - const currentTime = getCurrentTime(); - advanceTimers(currentTime); - const firstTask = peek(taskQueue); - return ( - (firstTask !== currentTask && - currentTask !== null && - firstTask !== null && - firstTask.callback !== null && - firstTask.startTime <= currentTime && - firstTask.expirationTime < currentTask.expirationTime) || - shouldYieldToHost() - ); -} - const unstable_requestPaint = requestPaint; export { @@ -422,7 +407,7 @@ export { unstable_cancelCallback, unstable_wrapCallback, unstable_getCurrentPriorityLevel, - unstable_shouldYield, + shouldYieldToHost as unstable_shouldYield, unstable_requestPaint, unstable_continueExecution, unstable_pauseExecution, diff --git a/packages/scheduler/src/__tests__/Scheduler-test.js b/packages/scheduler/src/__tests__/Scheduler-test.js index 202501512694c..e8da8ee53d69a 100644 --- a/packages/scheduler/src/__tests__/Scheduler-test.js +++ b/packages/scheduler/src/__tests__/Scheduler-test.js @@ -249,7 +249,7 @@ describe('Scheduler', () => { }); it( - 'continuations are interrupted by higher priority work scheduled ' + + 'continuations do not block higher priority work scheduled ' + 'inside an executing callback', () => { const tasks = [ @@ -272,8 +272,8 @@ describe('Scheduler', () => { Scheduler.unstable_yieldValue('High pri'); }); } - if (tasks.length > 0 && shouldYield()) { - Scheduler.unstable_yieldValue('Yield!'); + if (tasks.length > 0) { + // Return a continuation return work; } } @@ -283,9 +283,8 @@ describe('Scheduler', () => { 'A', 'B', 'Schedule high pri', - // Even though there's time left in the frame, the low pri callback - // should yield to the high pri callback - 'Yield!', + // The high pri callback should fire before the continuation of the + // lower pri work 'High pri', // Continue low pri work 'C', @@ -662,7 +661,7 @@ describe('Scheduler', () => { const [label, ms] = task; Scheduler.unstable_advanceTime(ms); Scheduler.unstable_yieldValue(label); - if (tasks.length > 0 && shouldYield()) { + if (tasks.length > 0) { return work; } } From 3f8115cdd1e6ba237619cf8a7d433900dcf413c2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 13 Aug 2020 18:02:32 -0400 Subject: [PATCH 12/12] Remove `didTimeout` check from work loop No longer need this, since we have starvation protection in userspace. This will also allow us to remove the concept from the Scheduler package, which is nice because `postTask` doesn't currently support it. --- .../src/ReactFiberWorkLoop.new.js | 15 +-------------- .../src/ReactFiberWorkLoop.old.js | 15 +-------------- .../__tests__/ReactSchedulerIntegration-test.js | 10 ++++++++-- 3 files changed, 10 insertions(+), 30 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index e35f87982daba..63f20c123a396 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -754,7 +754,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // This is the entry point for every concurrent task, i.e. anything that // goes through Scheduler. -function performConcurrentWorkOnRoot(root, didTimeout) { +function performConcurrentWorkOnRoot(root) { // Since we know we're in a React event, we can clear the current // event time. The next update will compute a new event time. currentEventTime = NoTimestamp; @@ -794,19 +794,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) { return null; } - // TODO: We only check `didTimeout` defensively, to account for a Scheduler - // bug where `shouldYield` sometimes returns `true` even if `didTimeout` is - // true, which leads to an infinite loop. Once the bug in Scheduler is - // fixed, we can remove this, since we track expiration ourselves. - if (didTimeout) { - // Something expired. Flush synchronously until there's no expired - // work left. - markRootExpired(root, lanes); - // This will schedule a synchronous callback. - ensureRootIsScheduled(root, now()); - return null; - } - let exitStatus = renderRootConcurrent(root, lanes); if ( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 0aabd21aab470..eab3949bde820 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -741,7 +741,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // This is the entry point for every concurrent task, i.e. anything that // goes through Scheduler. -function performConcurrentWorkOnRoot(root, didTimeout) { +function performConcurrentWorkOnRoot(root) { // Since we know we're in a React event, we can clear the current // event time. The next update will compute a new event time. currentEventTime = NoTimestamp; @@ -781,19 +781,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) { return null; } - // TODO: We only check `didTimeout` defensively, to account for a Scheduler - // bug where `shouldYield` sometimes returns `true` even if `didTimeout` is - // true, which leads to an infinite loop. Once the bug in Scheduler is - // fixed, we can remove this, since we track expiration ourselves. - if (didTimeout) { - // Something expired. Flush synchronously until there's no expired - // work left. - markRootExpired(root, lanes); - // This will schedule a synchronous callback. - ensureRootIsScheduled(root, now()); - return null; - } - let exitStatus = renderRootConcurrent(root, lanes); if ( diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js index c8c6b243f6d46..aca8679b7cf3b 100644 --- a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js @@ -446,8 +446,6 @@ describe( React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); - - React = require('react'); }); afterEach(() => { @@ -531,6 +529,14 @@ describe( // Expire the task Scheduler.unstable_advanceTime(10000); + // Scheduling a new update is a trick to force the expiration to kick + // in. We don't check if a update has been starved at the beginning of + // working on it, since there's no point — we're already working on it. + // We only check before yielding to the main thread (to avoid starvation + // by other main thread work) or when receiving an update (to avoid + // starvation by incoming updates). + ReactNoop.render(); + // Because the render expired, React should finish the tree without // consulting `shouldYield` again expect(Scheduler).toFlushExpired(['B', 'C']);