Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Make disableNewFiberFeatures = true the default when runnings tests
This exposed a few more cases that I missed.
  • Loading branch information
acdlite committed Jan 25, 2017
commit c0e5df66ecc4f98140d1089bc1feb4a78f2a32f5
1 change: 0 additions & 1 deletion scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,6 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
* should not crash encountering low-priority tree
* throws if non-element passed to top-level render
* throws if something other than false, null, or an element is returned from render
* still accepts arrays and iterables as host children

src/renderers/dom/shared/__tests__/CSSProperty-test.js
* should generate browser prefixes for its `isUnitlessNumber`
Expand Down
6 changes: 6 additions & 0 deletions scripts/jest/test-framework-setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ jest.mock('ReactDOMFeatureFlags', () => {
useFiber: flags.useFiber || !!process.env.REACT_DOM_JEST_USE_FIBER,
});
});
jest.mock('ReactFeatureFlags', () => {
const flags = require.requireActual('ReactFeatureFlags');
return Object.assign({}, flags, {
disableNewFiberFeatures: true,
});
});

// Error logging varies between Fiber and Stack;
// Rather than fork dozens of tests, mock the error-logging file by default.
Expand Down
15 changes: 15 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { ReactNodeList } from 'ReactTypes';
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
var ReactControlledComponent = require('ReactControlledComponent');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactFeatureFlags = require('ReactFeatureFlags');
var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
var ReactDOMFiberComponent = require('ReactDOMFiberComponent');
var ReactDOMFrameScheduling = require('ReactDOMFrameScheduling');
Expand All @@ -27,6 +28,8 @@ var ReactFiberReconciler = require('ReactFiberReconciler');
var ReactInputSelection = require('ReactInputSelection');
var ReactInstanceMap = require('ReactInstanceMap');
var ReactPortal = require('ReactPortal');
var { isValidElement } = require('ReactElement');


var findDOMNode = require('findDOMNode');
var invariant = require('invariant');
Expand All @@ -49,6 +52,7 @@ if (__DEV__) {
var { updatedAncestorInfo } = validateDOMNesting;
}


const DOCUMENT_NODE = 9;

ReactDOMInjection.inject();
Expand Down Expand Up @@ -352,6 +356,17 @@ var ReactDOM = {

render(element : ReactElement<any>, container : DOMContainerElement, callback: ?Function) {
validateContainer(container);

if (ReactFeatureFlags.disableNewFiberFeatures) {
// Top-level check occurs here instead of inside child reconciler because
// because requirements vary between renderers. E.g. React Art
// allows arrays.
invariant(
isValidElement(element),
'render(): Invalid component element.'
);
}

return renderSubtreeIntoContainer(null, element, container, callback);
},

Expand Down
77 changes: 40 additions & 37 deletions src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,17 @@ var ReactTestUtils = require('ReactTestUtils');

describe('ReactDOMFiber', () => {
var container;
var ReactFeatureFlags;

beforeEach(() => {
container = document.createElement('div');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

afterEach(() => {
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = true;
});

it('should render strings as children', () => {
Expand Down Expand Up @@ -1085,47 +1093,42 @@ describe('ReactDOMFiber', () => {
container
);
});
}
});

describe('disableNewFiberFeatures', () => {
var ReactFeatureFlags = require('ReactFeatureFlags');

beforeEach(() => {
ReactFeatureFlags.disableNewFiberFeatures = true;
});
// disableNewFiberFeatures currently defaults to true in test
describe('disableNewFiberFeatures', () => {
var container;
var ReactFeatureFlags;

afterEach(() => {
ReactFeatureFlags.disableNewFiberFeatures = false;
});
beforeEach(() => {
container = document.createElement('div');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = true;
});

it('throws if non-element passed to top-level render', () => {
const message = 'render(): Invalid component element.';
expect(() => ReactDOM.render(null, container)).toThrow(message, container);
expect(() => ReactDOM.render(undefined, container)).toThrow(message, container);
expect(() => ReactDOM.render(false, container)).toThrow(message, container);
expect(() => ReactDOM.render('Hi', container)).toThrow(message, container);
expect(() => ReactDOM.render(999, container)).toThrow(message, container);
expect(() => ReactDOM.render([<div />], container)).toThrow(message, container);
});
afterEach(() => {
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('throws if something other than false, null, or an element is returned from render', () => {
function Render(props) {
return props.children;
}
it('throws if non-element passed to top-level render', () => {
const message = 'render(): Invalid component element.';
expect(() => ReactDOM.render(null, container)).toThrow(message, container);
expect(() => ReactDOM.render(undefined, container)).toThrow(message, container);
expect(() => ReactDOM.render(false, container)).toThrow(message, container);
expect(() => ReactDOM.render('Hi', container)).toThrow(message, container);
expect(() => ReactDOM.render(999, container)).toThrow(message, container);
expect(() => ReactDOM.render([<div />], container)).toThrow(message, container);
});

expect(() => ReactDOM.render(<Render>Hi</Render>, container)).toThrow(/You may have returned undefined/);
expect(() => ReactDOM.render(<Render>{999}</Render>, container)).toThrow(/You may have returned undefined/);
expect(() => ReactDOM.render(<Render>[<div />]</Render>, container)).toThrow(/You may have returned undefined/);
});
it('throws if something other than false, null, or an element is returned from render', () => {
function Render(props) {
return props.children;
}

it('still accepts arrays and iterables as host children', () => {
function Render(props) {
return <div>{props.children}</div>;
}
ReactDOM.render(<Render>{['foo', 'bar']}</Render>, container);
expect(container.textContent).toEqual('foobar');
ReactDOM.render(<Render>{new Set(['foo', 'bar'])}</Render>, container);
expect(container.textContent).toEqual('foobar');
});
});
}
expect(() => ReactDOM.render(<Render>Hi</Render>, container)).toThrow(/You may have returned undefined/);
expect(() => ReactDOM.render(<Render>{999}</Render>, container)).toThrow(/You may have returned undefined/);
expect(() => ReactDOM.render(<Render>[<div />]</Render>, container)).toThrow(/You may have returned undefined/);
});
});
64 changes: 31 additions & 33 deletions src/renderers/shared/fiber/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ const {
NoEffect,
Placement,
Deletion,
Err,
} = ReactTypeOfSideEffect;

function coerceRef(current: ?Fiber, element: ReactElement) {
Expand Down Expand Up @@ -1127,41 +1126,40 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
}
}

if (returnFiber.tag === HostRoot) {
// Top-level only accepts elements or portals
invariant(
// If the root has an error effect, this is an intentional unmount.
// Don't throw an error.
returnFiber.effectTag & Err,
'render(): Invalid component element.'
);
} else {
switch (returnFiber.tag) {
case ClassComponent: {
if (__DEV__) {
const instance = returnFiber.stateNode;
if (instance.render._isMockFunction) {
// We allow auto-mocks to proceed as if they're
// returning null.
break;
}
switch (returnFiber.tag) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems unnecessary to fork this whole function now. Can't you just add this conditional down below behind a smaller if (ReactFeatureFlags.disableNewFiberFeatures) { instead of forking the whole function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forking the entire function was the only way I could get it down to a single check. But you're right that there's a lot of overlap and it's weird to fork the entire thing. I split it out into three checks instead.

case ClassComponent: {
if (__DEV__) {
const instance = returnFiber.stateNode;
if (instance.render._isMockFunction) {
// We allow auto-mocks to proceed as if they're
// returning null.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What you're really saying here is that they're allowed to return anything (including strings, arrays etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Looks like Stack only accepts undefined. I'll add a test.

if (renderedElement === undefined &&

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

break;
}
}
// Intentionally fall through to the next case, which handles both
// functions and classes
// eslint-disable-next-lined no-fallthrough
case FunctionalComponent: {
// Composites accept elements, portals, null, or false
const Component = returnFiber.type;
invariant(
newChild === null || newChild === false,
'%s.render(): A valid React element (or null) must be ' +
'returned. You may have returned undefined, an array or some ' +
'other invalid object.',
Component.displayName || Component.name || 'Component'
);
}
}
// Intentionally fall through to the next case, which handles both
// functions and classes
// eslint-disable-next-lined no-fallthrough
case FunctionalComponent: {
// Composites accept elements, portals, null, or false
const Component = returnFiber.type;
invariant(
newChild === null || newChild === false,
'%s(...): A valid React element (or null) must be ' +
'returned. You may have returned undefined, an array or some ' +
'other invalid object.',
Component.displayName || Component.name || 'Component'
);
}
}

if (typeof newChild === 'string' || typeof newChild === 'number') {
return placeSingleChild(reconcileSingleTextNode(
returnFiber,
currentFirstChild,
'' + newChild,
priority
));
}

if (isArray(newChild)) {
Expand Down
3 changes: 3 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,16 @@
var React;
var ReactNoop;
var ReactCoroutine;
var ReactFeatureFlags;

describe('ReactCoroutine', () => {
beforeEach(() => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactCoroutine = require('ReactCoroutine');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('should render a coroutine', () => {
Expand Down
4 changes: 4 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

describe('ReactIncremental', () => {
beforeEach(() => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');

ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('should render a simple component', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

describe('ReactIncrementalErrorHandling', () => {
beforeEach(() => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

function div(...children) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

describe('ReactIncrementalReflection', () => {
beforeEach(() => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('handles isMounted even when the initial render is deferred', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

describe('ReactIncrementalScheduling', () => {
beforeEach(() => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

function span(prop) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

describe('ReactIncrementalSideEffects', () => {
beforeEach(() => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

function normalizeCodeLocInfo(str) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

describe('ReactIncrementalUpdates', () => {
beforeEach(() => {
jest.resetModuleRegistry();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('applies updates in order of priority', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

// This is a new feature in Fiber so I put it in its own test file. It could
// probably move to one of the other test files once it is official.
Expand All @@ -21,6 +22,8 @@ describe('ReactTopLevelFragment', function() {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('should render a simple fragment at the top of a component', function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var React;
var ReactNoop;
var ReactFeatureFlags;

// This is a new feature in Fiber so I put it in its own test file. It could
// probably move to one of the other test files once it is official.
Expand All @@ -21,6 +22,8 @@ describe('ReactTopLevelText', () => {
jest.resetModules();
React = require('React');
ReactNoop = require('ReactNoop');
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('should render a component returning strings directly from render', () => {
Expand Down
Loading