Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change the argument passed to CallbackQueue.getPooled #10101

Merged
merged 10 commits into from
Jul 12, 2017
3 changes: 3 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ src/renderers/__tests__/ReactCompositeComponent-test.js
* prepares new child before unmounting old
* respects a shallow shouldComponentUpdate implementation
* does not do a deep comparison for a shallow shouldComponentUpdate implementation
* should call setState with the correct parameters

src/renderers/__tests__/ReactCompositeComponentDOMMinimalism-test.js
* should not render extra nodes for non-interpolated text
Expand Down Expand Up @@ -1972,6 +1973,7 @@ src/renderers/dom/test/__tests__/ReactTestUtils-test.js
* should enable rendering of cloned element
* should set the type of the event
* should work with renderIntoDocument
* should call setState with the correct parameters

src/renderers/native/__tests__/ReactNativeAttributePayload-test.js
* should work with simple example
Expand Down Expand Up @@ -2001,6 +2003,7 @@ src/renderers/native/__tests__/ReactNativeMount-test.js
* should be able to create and update a native component
* returns the correct instance and calls it in the callback
* renders and reorders children
* calls setState with the correct parameters

src/renderers/native/__tests__/createReactNativeComponentClass-test.js
* should register viewConfigs
Expand Down
16 changes: 16 additions & 0 deletions src/renderers/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1472,4 +1472,20 @@ describe('ReactCompositeComponent', () => {
instance.setState(getInitialState());
expect(renderCalls).toBe(3);
});

it('should call setState with the correct parameters', () => {
let mockArg = 'the wrong value';
let mockCallback = arg => (mockArg = arg);
class Component extends React.Component {
componentDidMount() {
this.setState({}, mockCallback);
}
render() {
return false;
}
}

ReactTestUtils.renderIntoDocument(<Component />);
expect(mockArg).toBeUndefined();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ function ReactReconcileTransaction(useCreateElement) {
// accessible and defaults to false when `ReactDOMComponent` and
// `ReactDOMTextComponent` checks it in `mountComponent`.`
this.renderToStaticMarkup = false;
this.reactMountReady = CallbackQueue.getPooled(null);
this.reactMountReady = CallbackQueue.getPooled();
this.useCreateElement = useCreateElement;
}

Expand Down
16 changes: 16 additions & 0 deletions src/renderers/dom/test/__tests__/ReactTestUtils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -844,4 +844,20 @@ describe('ReactTestUtils', () => {
);
});
});

it('should call setState with the correct parameters', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change this to should call setState callback with no arguments (and others too)

let mockArg = 'the wrong value';
let mockCallback = arg => (mockArg = arg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe let's assert arguments.length is 0 instead of checking first argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I tried this but npm test causes the length of arguments to be 1, [ undefined ]. But node ./scripts/fiber/record-tests has the length of the arguments as 0, [ ]

So it seems like checking the first argument is the only way to go that I see

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you see where that undefined comes from? I’d like to understand it.

class Component extends React.Component {
componentDidMount() {
this.setState({}, mockCallback);
}
render() {
return false;
}
}

ReactTestUtils.renderIntoDocument(<Component />);
expect(mockArg).toBeUndefined();
});
});
2 changes: 1 addition & 1 deletion src/renderers/native/ReactNativeReconcileTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ if (__DEV__) {
*/
function ReactNativeReconcileTransaction() {
this.reinitializeTransaction();
this.reactMountReady = CallbackQueue.getPooled(null);
this.reactMountReady = CallbackQueue.getPooled();
}

var Mixin = {
Expand Down
16 changes: 16 additions & 0 deletions src/renderers/native/__tests__/ReactNativeMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,20 @@ describe('ReactNative', () => {
ReactNative.render(<Component chars={after} />, 11);
expect(UIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
});

it('calls setState with the correct parameters', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a similar test to ReactTestUtils-test for shallow renderer, and to ReactCompositeComponent-test for DOM renderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

var mockArg = 'the wrong value';
var mockCallback = arg => (mockArg = arg);
class Component extends React.Component {
componentDidMount() {
this.setState({}, mockCallback);
}
render() {
return false;
}
}

ReactNative.render(<Component />, 11);
expect(mockArg).toBeUndefined();
});
});