Skip to content

update node.js version for CI to 16 #23243

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
sunderls wants to merge 2 commits into from
Closed

update node.js version for CI to 16 #23243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84e84d2
update node.js version for CI
sunderls Feb 6, 2022
45564d3
use __DEV__
sunderls Feb 7, 2022
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
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: 2.1

aliases:
- &docker
- image: cimg/openjdk:17.0.0-node
- image: cimg/openjdk:17.0.2-node
Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022

Choose a reason for hiding this comment

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

Screenshot 2022-02-06 at 22 08 17


- &environment
TZ: /usr/share/zoneinfo/America/Los_Angeles
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,9 +503,13 @@ function runActTests(label, render, unmount, rerender) {
spyOnDevAndProd(console, 'error');
// let's try to cheat and spin off a 'thread' with an act call
(async () => {
await act(async () => {
await sleep(50);
});
try {
await act(async () => {
await sleep(50);
});
} catch (e) {
// suppress ERR_UNHANDLED_REJECTION
}
Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022

Choose a reason for hiding this comment

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

before

Screenshot 2022-02-07 at 19 43 56

from CI
Screenshot 2022-02-07 at 20 08 51

after

Screenshot 2022-02-07 at 19 48 13

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Feb 7, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022

Choose a reason for hiding this comment

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

  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?

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Feb 7, 2022

Choose a reason for hiding this comment

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

can we catch that specific error?

Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022

Choose a reason for hiding this comment

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

Ah, got you!

Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022

Choose a reason for hiding this comment

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

(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);
        });
    })();
  }

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Feb 7, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Feb 7, 2022

Choose a reason for hiding this comment

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

sounds reasonable

})();

await act(async () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
store.set({});
});
expect(container.textContent).toEqual(
"Cannot read property 'toUpperCase' of undefined",
Copy link
Copy Markdown
Contributor Author

@sunderls sunderls Feb 7, 2022

Choose a reason for hiding this comment

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

before

Screenshot 2022-02-06 at 22 07 17

TypeError message is changed from node.js 16

"Cannot read properties of undefined (reading 'toUpperCase')",
);
});

Expand Down Expand Up @@ -935,7 +935,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
store.set({});
});
expect(container.textContent).toEqual(
"Cannot read property 'trim' of undefined",
"Cannot read properties of undefined (reading 'trim')",
);
});
});
Expand Down