Skip to content

Warn if ReactNoop.act is not awaited #14955

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.

Sign up for GitHub

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

Closed
acdlite wants to merge 1 commit into from
Closed

Warn if ReactNoop.act is not awaited #14955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
101 changes: 57 additions & 44 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
let didStop: boolean = false;
let expectedNumberOfYields: number = -1;

let actDepth = 0;

function shouldYield() {
if (
expectedNumberOfYields !== -1 &&
Expand All @@ -553,8 +555,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
return false;
}

function flushAll(): Array<mixed> {
yieldedValues = [];
function flushAll() {
while (scheduledCallback !== null) {
const cb = scheduledCallback;
scheduledCallback = null;
Expand All @@ -563,15 +564,14 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
scheduledCallbackTimeout < elapsedTimeInMs;
cb(didTimeout);
}
const values = yieldedValues;
yieldedValues = [];
return values;
}

function flushNumberOfYields(count: number): Array<mixed> {
expectedNumberOfYields = count;
function flushNumberOfYields(count: number) {
if (expectedNumberOfYields !== -1) {
throw new Error('Cannot nest flushNumberOfYields');
}
expectedNumberOfYields = yieldedValues.length + count;
didStop = false;
yieldedValues = [];
try {
while (scheduledCallback !== null && !didStop) {
const cb = scheduledCallback;
Expand All @@ -581,11 +581,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
scheduledCallbackTimeout < elapsedTimeInMs;
cb(didTimeout);
}
return yieldedValues;
} finally {
expectedNumberOfYields = -1;
didStop = false;
yieldedValues = [];
}
}

Expand Down Expand Up @@ -769,7 +767,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
unstable_flushNumberOfYields: flushNumberOfYields,
unstable_clearYields: clearYields,

flushNextYield(): Array<mixed> {
flushNextYield(): void {
return flushNumberOfYields(1);
},

Expand Down Expand Up @@ -805,17 +803,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
}
},

expire(ms: number): Array<mixed> {
expire(ms: number): void {
ReactNoop.advanceTime(ms);
return ReactNoop.flushExpired();
ReactNoop.flushExpired();
},

advanceTime(ms: number): void {
elapsedTimeInMs += ms;
},

flushExpired(): Array<mixed> {
return flushNumberOfYields(0);
flushExpired(): void {
flushNumberOfYields(0);
},

yield: yieldValue,
Expand All @@ -832,38 +830,52 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

interactiveUpdates: NoopRenderer.interactiveUpdates,

// maybe this should exist only in the test file
act(callback: () => void): Thenable {
// note: keep these warning messages in sync with
// ReactTestRenderer.js and ReactTestUtils.js
let result = NoopRenderer.batchedUpdates(callback);
actDepth++;

const result = NoopRenderer.batchedUpdates(callback);

let didAwait = false;

if (__DEV__) {
if (result !== undefined) {
let addendum;
if (result !== null && typeof result.then === 'function') {
addendum =
"\n\nIt looks like you wrote ReactNoop.act(async () => ...) or returned a Promise from it's callback. " +
'Putting asynchronous logic inside ReactNoop.act(...) is not supported.\n';
} else {
addendum = ' You returned: ' + result;
}
warningWithoutStack(
false,
'The callback passed to ReactNoop.act(...) function must not return anything.%s',
addendum,
);
}
}
ReactNoop.flushPassiveEffects();
// we want the user to not expect a return,
// but we want to warn if they use it like they can await on it.
return {
then() {
if (__DEV__) {
Promise.resolve()
.then(() => {})
.then(() => {
warningWithoutStack(
false,
'Do not await the result of calling ReactNoop.act(...), it is not a Promise.',
didAwait,
'You called act() without awaiting its result.',
);
});
}

return {
then(resolve, reject) {
if (didAwait) {
return;
}
didAwait = true;
if (
result !== undefined &&
result !== null &&
typeof result.then === 'function'
) {
result.then(() => {
actDepth--;
if (actDepth === 0) {
// TODO: Should keep flushing until queue is empty
ReactNoop.unstable_flushWithoutYielding();
Copy link
Contributor

@threepointone threepointone Feb 26, 2019

Choose a reason for hiding this comment

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

this would simplify tests too (which I assume was the reason you did this). til, thanks!

ReactNoop.flushPassiveEffects();
}
resolve();
}, reject);
} else {
actDepth--;
if (actDepth === 0) {
// TODO: Should keep flushing until queue is empty
ReactNoop.unstable_flushWithoutYielding();
ReactNoop.flushPassiveEffects();
}
resolve();
}
},
};
Expand Down Expand Up @@ -997,7 +1009,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
_next: null,
};
root.firstBatch = batch;
const actual = flushAll();
flushAll();
const actual = clearYields();
expect(actual).toEqual(expectedFlush);
return (expectedCommit: Array<mixed>) => {
batch._defer = false;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('ReactExpiration', () => {
// work signals to the renderer that the event has ended.
ReactNoop.advanceTime(2000);
// Don't advance time by enough to expire the first update.
expect(ReactNoop.flushExpired()).toEqual([]);
ReactNoop.flushExpired();
expect(ReactNoop.getChildren()).toEqual([]);
// Schedule another update.
ReactNoop.render(<Text text="B" />);
Expand Down Expand Up @@ -121,7 +121,7 @@ describe('ReactExpiration', () => {
// work signals to the renderer that the event has ended.
ReactNoop.advanceTime(2000);
// Don't advance time by enough to expire the first update.
expect(ReactNoop.flushExpired()).toEqual([]);
ReactNoop.flushExpired();
expect(ReactNoop.getChildren()).toEqual([]);
// Schedule another update.
ReactNoop.render(<Text text="B" />);
Expand Down
Loading