update node.js version for CI to 16

[sunderls]

Summary

PR again for #23236 (one test fix missed for prev PR)

this diff bumps node.js version used CI to 16 to improve the developer experience.
Accordingly some tests needs to be modified.

How did you test this change?

> yarn prettier
> yarn lint
> yarn flow
> yarn test
> yarn test --prod

[sunderls]

[sunderls]

before

from CI

after

[gaearon]

Catch-all is risky. How can we narrow it down? Also, why is it needed now exactly?

[sunderls]

1. in act() an error is throw for prod which causes the rejection https://github.com/facebook/react/blob/main/packages/react/src/ReactAct.js#L153

2. in node.js 15 default behavior for uncaught promise reject was changed from warn to throw, which causes the trouble.
   process: Change default --unhandled-rejections=throw nodejs/node#33021. but previously we are using node.js 14, so not an issue.

I guess this fix is fine ? since it should not throw for DEV, do you suggest any other possible fix directions?

[gaearon]

can we catch that specific error?

[sunderls]

Ah, got you!

[sunderls]

(async () => {
    try {
      await act(async () => {
        await sleep(50);
      });
    } catch (e) {
      // suppress ERR_UNHANDLED_REJECTION from act()
      if (e.message !== 'act(...) is not supported in production builds of React.') {
        throw e
      }
    }
  })();

this works, or we just guard it with __DEV__?

if (__DEV__) {
    (async () => {
        await act(async () => {
          await sleep(50);
        });
    })();
  }

[gaearon]

hmm. there is // @gate __DEV__ above. shouldn't this be enough?

[sunderls]

Aha, things becoming interesting.
// @gate __DEV__ doesn't skip test in non-Dev, it still runs the test but expects it to fail.

react/scripts/jest/setupTests.js

Lines 270 to 287 in cd4eb11

	global._test_gate = (gateFn, testName, callback) => {
	let shouldPass;
	try {
	const flags = getTestFlags();
	shouldPass = gateFn(flags);
	} catch (e) {
	test(testName, () => {
	throw e;
	});
	return;
	}
	if (shouldPass) {
	test(testName, callback);
	} else {
	test(`[GATED, SHOULD FAIL] ${testName}`, () =>
	expectTestToFail(callback, gatedErrorMessage));
	}
	};

The test is still run

react/scripts/jest/setupTests.js

Line 249 in cd4eb11

const maybePromise = callback();

and the exception is not caught, trouble occurs.

since the assertions for this test are under __DEV__

react/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js

Lines 516 to 525 in cd4eb11

	if (__DEV__) {
	expect(console.error).toHaveBeenCalledTimes(2);
	expect(console.error.calls.argsFor(0)[0]).toMatch(
	'You seem to have overlapping act() calls',
	);
	expect(console.error.calls.argsFor(1)[0]).toMatch(
	'You seem to have overlapping act() calls',
	);
	}
	});

, I guess we can just use __DEV__ as well?

[gaearon]

sounds reasonable

[sunderls]

before

TypeError message is changed from node.js 16

[gaearon]

Do we understand why it failed on main but not on PR CI previously? What was the difference?

[sunderls]

There is one mysterious failure on devtool e2e test https://app.circleci.com/pipelines/github/facebook/react/23442/workflows/2d3aa996-abb7-48c7-a8f7-256657b8b1bd/jobs/431345

I couldn't figure out the possible cause on node.js version. since all tests pass for my local env.

I guess the failure is transient , or we can just fix in another chance?

@lunaruan do you know what might be the cause?