@samitouri / QOS-React / commits / 83a536e622

Fix "null" instead of the component stack in a warning (#10915)

* Add a failing test that prints "null" in the warning instead of the stack * Don't reset the current fiber during reconciliation It should only be reset when we stop doing work. Otherwise, any warnings after the reset will lose the component stack. The reset in getMaskedContext() was completely unnecessary. It is always called with the same fiber as the current work in progress. Therefore, I add a DEV-only warning assertion to ensure we don't regress, and remove the reset. The reset in processChildContext() is necessary because it can be called outside of reconciliation. Unfortunately, we have to keep this hack in until we can remove unstable_renderSubtreeIntoContainer(). To work around it, I restore the previous fiber instead of resetting. * Decouple setting the current fiber and phase These are two distinct actions. This helps make it clearer when we're actually changing the current pointer. I'm also removing an overengineered hack I previously added for unstable_renderSubtreeIntoContainer. It's not necessary now that we don't null the pointer all the time. This makes the code more straightforward. * Centralize the pointer updates in the scheduler

Dan Abramov committed Sep 28, 2017 at 20:40 UTC 83a536e62264b7baf0bb5d83835821e6f4c45006

7 files changed +72 -47

src/renderers/__tests__/ReactStatelessComponent-test.js

+37

@@ -295,6 +295,43 @@ describe('ReactStatelessComponent', () => {

295 console.error.calls.reset();

296 });

297

298 + // This guards against a regression caused by clearing the current debug fiber.

299 + // https://github.com/facebook/react/issues/10831

300 + it('should warn when giving a function ref with context', () => {

301 + spyOn(console, 'error');

302 +

303 + function Child() {

304 + return null;

305 + }

306 + Child.contextTypes = {

307 + foo: PropTypes.string,

308 + };

309 +

310 + class Parent extends React.Component {

311 + static childContextTypes = {

312 + foo: PropTypes.string,

313 + };

314 + getChildContext() {

315 + return {

316 + foo: 'bar',

317 + };

318 + }

319 + render() {

320 + return <Child ref={function() {}} />;

321 + }

322 + }

323 +

324 + ReactTestUtils.renderIntoDocument(<Parent />);

325 + expectDev(console.error.calls.count()).toBe(1);

326 + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(

327 + 'Warning: Stateless function components cannot be given refs. ' +

328 + 'Attempts to access this ref will fail.\n\nCheck the render method ' +

329 + 'of `Parent`.\n' +

330 + ' in Child (at **)\n' +

331 + ' in Parent (at **)',

332 + );

333 + });

334 +

335 it('should provide a null ref', () => {

336 function Child() {

337 return <div />;

src/renderers/shared/fiber/ReactDebugCurrentFiber.js

+6 -1

@@ -55,9 +55,13 @@ function resetCurrentFiber() {

55 ReactDebugCurrentFiber.phase = null;

56 }

58 -function setCurrentFiber(fiber: Fiber | null, phase: LifeCyclePhase | null) {

58 +function setCurrentFiber(fiber: Fiber) {

59 ReactDebugCurrentFrame.getCurrentStack = getCurrentFiberStackAddendum;

60 ReactDebugCurrentFiber.current = fiber;

61 + ReactDebugCurrentFiber.phase = null;

62 +}

63 +

64 +function setCurrentPhase(phase: LifeCyclePhase | null) {

65 ReactDebugCurrentFiber.phase = phase;

66 }

@@ -66,6 +70,7 @@ var ReactDebugCurrentFiber = {

70 phase: (null: LifeCyclePhase | null),

71 resetCurrentFiber,

72 setCurrentFiber,

73 + setCurrentPhase,

74 getCurrentFiberOwnerName,

75 getCurrentFiberStackAddendum,

76 };

src/renderers/shared/fiber/ReactFiberBeginWork.js

+4 -8

@@ -207,9 +207,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

207

208 if (__DEV__) {

209 ReactCurrentOwner.current = workInProgress;

210 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, 'render');

210 + ReactDebugCurrentFiber.setCurrentPhase('render');

211 nextChildren = fn(nextProps, context);

212 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, null);

212 + ReactDebugCurrentFiber.setCurrentPhase(null);

213 } else {

214 nextChildren = fn(nextProps, context);

215 }

@@ -281,9 +281,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

281 ReactCurrentOwner.current = workInProgress;

282 let nextChildren;

283 if (__DEV__) {

284 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, 'render');

284 + ReactDebugCurrentFiber.setCurrentPhase('render');

285 nextChildren = instance.render();

286 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, null);

286 + ReactDebugCurrentFiber.setCurrentPhase(null);

287 } else {

288 nextChildren = instance.render();

289 }

@@ -725,10 +725,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

725 return bailoutOnLowPriority(current, workInProgress);

726 }

727

728 - if (__DEV__) {

729 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, null);

730 - }

731 -

728 switch (workInProgress.tag) {

729 case IndeterminateComponent:

730 return mountIndeterminateComponent(

src/renderers/shared/fiber/ReactFiberCompleteWork.js

-8

@@ -42,10 +42,6 @@ var {

42 var {Placement, Ref, Update} = ReactTypeOfSideEffect;

43 var {OffscreenPriority} = ReactPriorityLevel;

45 -if (__DEV__) {

46 - var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');

47 -}

48 -

45 var invariant = require('fbjs/lib/invariant');

47 module.exports = function<T, P, I, TI, PI, C, CX, PL>(

@@ -187,10 +183,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

183 workInProgress: Fiber,

184 renderPriority: PriorityLevel,

185 ): Fiber | null {

190 - if (__DEV__) {

191 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, null);

192 - }

193 -

186 // Get the latest props.

187 let newProps = workInProgress.pendingProps;

188 if (newProps === null) {

src/renderers/shared/fiber/ReactFiberContext.js

+9 -22

@@ -89,7 +89,6 @@ exports.getMaskedContext = function(

90 if (__DEV__) {

91 const name = getComponentName(workInProgress) || 'Unknown';

92 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, null);

92 checkPropTypes(

93 contextTypes,

94 context,

@@ -97,7 +96,6 @@ exports.getMaskedContext = function(

96 name,

97 ReactDebugCurrentFiber.getCurrentFiberStackAddendum,

98 );

100 - ReactDebugCurrentFiber.resetCurrentFiber();

99 }

100

101 // Cache unmasked context so we can avoid recreating masked context unless necessary.

@@ -153,11 +151,7 @@ exports.pushTopLevelContextObject = function(

151 push(didPerformWorkStackCursor, didChange, fiber);

152 };

153

156 -function processChildContext(

157 - fiber: Fiber,

158 - parentContext: Object,

159 - isReconciling: boolean,

160 -): Object {

154 +function processChildContext(fiber: Fiber, parentContext: Object): Object {

155 const instance = fiber.stateNode;

156 const childContextTypes = fiber.type.childContextTypes;

157

@@ -184,11 +178,11 @@ function processChildContext(

178

179 let childContext;

180 if (__DEV__) {

187 - ReactDebugCurrentFiber.setCurrentFiber(fiber, 'getChildContext');

181 + ReactDebugCurrentFiber.setCurrentPhase('getChildContext');

182 startPhaseTimer(fiber, 'getChildContext');

183 childContext = instance.getChildContext();

184 stopPhaseTimer();

191 - ReactDebugCurrentFiber.resetCurrentFiber();

185 + ReactDebugCurrentFiber.setCurrentPhase(null);

186 } else {

187 childContext = instance.getChildContext();

188 }

@@ -202,21 +196,18 @@ function processChildContext(

196 }

197 if (__DEV__) {

198 const name = getComponentName(fiber) || 'Unknown';

205 - // We can only provide accurate element stacks if we pass work-in-progress tree

206 - // during the begin or complete phase. However currently this function is also

207 - // called from unstable_renderSubtree legacy implementation. In this case it unsafe to

208 - // assume anything about the given fiber. We won't pass it down if we aren't sure.

209 - // TODO: remove this hack when we delete unstable_renderSubtree in Fiber.

210 - const workInProgress = isReconciling ? fiber : null;

211 - ReactDebugCurrentFiber.setCurrentFiber(workInProgress, null);

199 checkPropTypes(

200 childContextTypes,

201 childContext,

202 'child context',

203 name,

204 + // In practice, there is one case in which we won't get a stack. It's when

205 + // somebody calls unstable_renderSubtreeIntoContainer() and we process

206 + // context from the parent component instance. The stack will be missing

207 + // because it's outside of the reconciliation, and so the pointer has not

208 + // been set. This is rare and doesn't matter. We'll also remove that API.

209 ReactDebugCurrentFiber.getCurrentFiberStackAddendum,

210 );

219 - ReactDebugCurrentFiber.resetCurrentFiber();

211 }

212

213 return {...parentContext, ...childContext};

@@ -264,11 +255,7 @@ exports.invalidateContextProvider = function(

255 // Merge parent and own context.

256 // Skip this if we're not updating due to sCU.

257 // This avoids unnecessarily recomputing memoized values.

267 - const mergedContext = processChildContext(

268 - workInProgress,

269 - previousContext,

270 - true,

271 - );

258 + const mergedContext = processChildContext(workInProgress, previousContext);

259 instance.__reactInternalMemoizedMergedChildContext = mergedContext;

260

261 // Replace the old (or empty) context with the new one.

src/renderers/shared/fiber/ReactFiberReconciler.js

+1 -1

@@ -181,7 +181,7 @@ export type Reconciler<C, I, TI> = {

181 getContextForSubtree._injectFiber(function(fiber: Fiber) {

182 const parentContext = findCurrentUnmaskedContext(fiber);

183 return isContextProvider(fiber)

184 - ? processChildContext(fiber, parentContext, false)

184 + ? processChildContext(fiber, parentContext)

185 : parentContext;

186 });

187

src/renderers/shared/fiber/ReactFiberScheduler.js

+15 -7

@@ -317,7 +317,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

317 function commitAllHostEffects() {

318 while (nextEffect !== null) {

319 if (__DEV__) {

320 - ReactDebugCurrentFiber.setCurrentFiber(nextEffect, null);

320 + ReactDebugCurrentFiber.setCurrentFiber(nextEffect);

321 recordEffect();

322 }

323

@@ -615,7 +615,13 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

615 // means that we don't need an additional field on the work in

616 // progress.

617 const current = workInProgress.alternate;

618 + if (__DEV__) {

619 + ReactDebugCurrentFiber.setCurrentFiber(workInProgress);

620 + }

621 const next = completeWork(current, workInProgress, nextPriorityLevel);

622 + if (__DEV__) {

623 + ReactDebugCurrentFiber.resetCurrentFiber();

624 + }

625

626 const returnFiber = workInProgress.return;

627 const siblingFiber = workInProgress.sibling;

@@ -706,8 +712,12 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

712 // See if beginning this work spawns more work.

713 if (__DEV__) {

714 startWorkTimer(workInProgress);

715 + ReactDebugCurrentFiber.setCurrentFiber(workInProgress);

716 }

717 let next = beginWork(current, workInProgress, nextPriorityLevel);

718 + if (__DEV__) {

719 + ReactDebugCurrentFiber.resetCurrentFiber();

720 + }

721 if (__DEV__ && ReactFiberInstrumentation.debugTool) {

722 ReactFiberInstrumentation.debugTool.onBeginWork(workInProgress);

723 }

@@ -718,9 +728,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

728 }

729

730 ReactCurrentOwner.current = null;

721 - if (__DEV__) {

722 - ReactDebugCurrentFiber.resetCurrentFiber();

723 - }

731

732 return next;

733 }

@@ -735,8 +742,12 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

742 // See if beginning this work spawns more work.

743 if (__DEV__) {

744 startWorkTimer(workInProgress);

745 + ReactDebugCurrentFiber.setCurrentFiber(workInProgress);

746 }

747 let next = beginFailedWork(current, workInProgress, nextPriorityLevel);

748 + if (__DEV__) {

749 + ReactDebugCurrentFiber.resetCurrentFiber();

750 + }

751 if (__DEV__ && ReactFiberInstrumentation.debugTool) {

752 ReactFiberInstrumentation.debugTool.onBeginWork(workInProgress);

753 }

@@ -747,9 +758,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

758 }

759

760 ReactCurrentOwner.current = null;

750 - if (__DEV__) {

751 - ReactDebugCurrentFiber.resetCurrentFiber();

752 - }

761

762 return next;

763 }