[risk=low][RW-9199] Upgrade React to v17 #7167

jmthibault79 · 2022-11-16T22:02:48Z

Updated to React 17.
React 17 makes changes to how event delegation works.
This change to event delegation necessitated that we upgrade to at least Prime React 7, because this is the earliest version that handles event delegation in a way that is compatible with React 17.

Updating our version of PrimeReact involved updating the Growl component to the Toast Component, changing how dropdown's attach their options to the dom, and adjusting many tests to account for selector discrepancies.

PR checklist

…dency

ui/package.json

ui/src/app/utils/runtime-utils.spec.tsx

ui/yarn.lock

jmthibault79 · 2022-11-17T15:29:17Z

e2e failures make me suspicious that the react-switch change is the problem. I'll try reverting.

jmthibault79 · 2022-11-17T15:49:56Z

our original version of react-switch (4.1.0) continues to work fine here, so hopefully this resolves the e2e failures.

jmthibault79 · 2022-11-17T16:00:57Z

6 e2e failures... trying again but not feeling confident

jmthibault79 · 2022-11-17T17:01:16Z

4 e2e failures:

3 of

TimeoutError: waiting for XPath `//*[contains(concat(" ", normalize-space(@class), " "), " p-tieredmenu ") or
contains(concat(" ", normalize-space(@class), " "), " p-menu ")][contains(concat(" ", normalize-space(@class), " "), " 
p-menu-overlay-visible ")][.//*[@role="menuitem"]]` failed: timeout 60000ms exceeded

and 1 similar error:

Error: TimeoutError: waiting for XPath `//*[contains(concat(" ", normalize-space(@class), " "), " p-tieredmenu ") or
contains(concat(" ", normalize-space(@class), " "), " p-menu ")][contains(concat(" ", normalize-space(@class), " "), " 
p-menu-overlay-visible ")][.//*[@role="menuitem"]]//*[not(contains(concat(" ", normalize-space(@class), " "), " menuitem-header "))]/*[@role="menuitem" and normalize-space()="Save as"]` failed: timeout 10000ms exceeded

I'm assuming these are real errors, and reverting to Draft

jmthibault79 · 2022-11-21T14:32:45Z

The e2e tests have caught a product failure: the Add Criteria button no longer works with this upgrade.

From some brief debugging, I see that cohort-criteria-menu's onClickOutside() is getting triggered, and this closes the menu immediately after opening it. Why?

jmthibault79 · 2022-11-21T14:54:14Z

This might be why: https://blog.saeloun.com/2021/07/08/react-17-event-delagation.html

jmthibault79 · 2023-01-13T21:12:24Z

e2ev2/tests/workspace-analysis.test.js

-  * */
-  await applicationListDropdown.evaluate(b => b.click())
+  const applicationListDropdown = await page.waitForSelector('#application-list-dropdown')
+  await applicationListDropdown.click()


is this change applicable elsewhere in e2ev2?

No, I do not believe so. It seems like the user-create-test was doing something similar. I ended up switching this to the approach that I found there, and it worked. Keeping that change for consistency.

jmthibault79

Github won't let me Approve because I'm still the author of this PR :)

But everything I've seen makes sense. Thanks for putting in all this work!

When making side-by-side comparisons I do still see some styling differences (though some are improvements) and I have some ideas about how to proceed there, but it's a longer discussion. I would prefer not to merge just yet.

jmthibault79 · 2023-01-18T13:50:09Z

ui/yarn.lock

-  version "16.9.14"
-  resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-16.9.14.tgz#674b8f116645fe5266b40b525777fc6bb8eb3bcd"
-  integrity sha512-FIX2AVmPTGP30OUJ+0vadeIFJJ07Mh1m+U0rxfgyW34p3rTlXI+nlenvAxNn4BP36YyI9IJ/+UJ7Wu22N1pI7A==
+"@types/react-dom@^17":


these resolutions are looking much better, thanks

…n local and test.

Peter asked me to do so

dmohs

Solid.

dmohs · 2023-01-19T16:05:02Z

e2ev2/src/circle-run-tests.sh

@@ -31,8 +31,7 @@ set +e
 export FAILED_TESTS_LOG=failed-tests.txt
 # This should do the right thing. If $FAILED_TESTS is empty, nothing is specified, so Jest runs
 # all tests.
-yarn test $FAILED_TESTS \
-  --reporters=jest-silent-reporter --reporters=./src/failure-reporter.js


Failure reporter is needed to ensure re-runs don't run every test again. I think you should revert this and we can isolate it into its own PR.

dmohs · 2023-01-19T16:10:35Z

e2ev2/tests/user-create.test.js

+  await page.waitForFunction(() =>
+      document.querySelector('[role="button"][aria-label="Next"]').style.cursor !== 'not-allowed')
+
+  await page.click( '[role="button"][aria-label="Next"]')


Consider:

Suggested change

await page.waitForFunction(() =>

document.querySelector('[role="button"][aria-label="Next"]').style.cursor !== 'not-allowed')

await page.click( '[role="button"][aria-label="Next"]')

await Promise.resolve('[role="button"][aria-label="Next"]').then(sel => {

page.waitForFunction(sel => document.querySelector(sel).style.cursor !== 'not-allowed', sel)

return sel

}).then(sel => page.click(sel))

I like it. Thanks.

dmohs · 2023-01-19T16:48:15Z

ui/src/app/pages/login/account-creation/account-creation-institution.spec.tsx

@@ -133,6 +133,8 @@ it('should reset role value & options when institution is selected', async () =>
    originalEvent: undefined,
    value: 'Broad',
    target: { name: '', id: '', value: 'Broad' },
+    stopPropagation: () => {},
+    preventDefault: () => {},


Consider something like:

const eventDefaults = {stopPropagation: () => undefined, preventDefault: () => undefined} // ... institutionDropdown.props.onChange({...eventDefaults, originalEvent: undefined, value: 'Broad', target: { name: '', id: '', value: 'Broad' }, })

jmthibault79 requested a review from evrii November 16, 2022 22:02

jmthibault79 marked this pull request as ready for review November 16, 2022 22:06

jmthibault79 added 4 commits November 17, 2022 09:13

upgrade React to v17

46dbf4b

fix tests

a49cc11

collapse duplicate resolutions

737f584

post rebase resolutions

aeb22a6

jmthibault79 force-pushed the react-17 branch from 39241da to aeb22a6 Compare November 17, 2022 14:20

bump react-switch to the earliest version with a react-16+ peer depen…

03476c5

…dency

jmthibault79 commented Nov 17, 2022

View reviewed changes

ui/package.json Show resolved Hide resolved

fix test

d303f04

jmthibault79 commented Nov 17, 2022

View reviewed changes

ui/src/app/utils/runtime-utils.spec.tsx Show resolved Hide resolved

jmthibault79 commented Nov 17, 2022

View reviewed changes

ui/yarn.lock Outdated Show resolved Hide resolved

Peter-Lavigne previously approved these changes Nov 17, 2022

View reviewed changes

revert react-switch

3d374cb

jmthibault79 marked this pull request as draft November 17, 2022 17:02

jmthibault79 changed the title ~~[risk=low][no ticket] Upgrade React to v17~~ [risk=low][RW-9199] Upgrade React to v17 Nov 18, 2022

evrii added 4 commits November 30, 2022 14:58

Merge remote-tracking branch 'origin/main' into react-17

1325f99

Change listener to be attached to root node rather than document.

6f0df4c

Change listeners to be attached to root node rather than document.

9428a36

An attempt to resolve a problem with PrimeReact's Menu component.

d5fdb3b

jmthibault79 mentioned this pull request Dec 2, 2022

Joel/react 17 #7209

Closed

10 tasks

evrii added 3 commits December 16, 2022 14:38

Update Prime React Version

d65829b

Backing out PrimeReact Changes in order to deal with Clarity first.

fe01139

Phasing out Clarity Ui styling

0c6df26

evrii marked this pull request as ready for review January 13, 2023 20:52

jmthibault79 commented Jan 13, 2023

View reviewed changes

evrii added 5 commits January 13, 2023 16:33

Regenerated with yarn.

02aabe9

Regenerated with yarn.

5494d6e

Regenerated with yarn.

8077a18

Updating click functionality to be consistent with user-create-test.

bed8940

Updating click functionality to be consistent with user-create-test.

c7107b4

jmthibault79 commented Jan 18, 2023

View reviewed changes

evrii added 4 commits January 18, 2023 10:09

Updating click functionality for account creation. Disabled captcha i…

4421875

…n local and test.

Testing e2ev2 reporting.

e3287dd

Testing e2ev2 reporting.

11aab94

Updated spacing of cohort save menu.

1099752

Peter-Lavigne removed their request for review January 19, 2023 15:33

jmthibault79 requested a review from Peter-Lavigne January 19, 2023 15:43

dmohs approved these changes Jan 19, 2023

View reviewed changes

evrii added 11 commits January 19, 2023 14:23

Updated cohort review to work with PrimeReact update.

0bdb113

Fixed tab styling issues.

076c9cc

Fixed overlay panel appendTo elements.

128cfc5

Updated how we look for the next button during account creation.

0d4626e

Consolidated event defaults in test.

269345f

Reverted failed test change.

11be103

Fixed styling issues.

f3b8c73

Fixed styling issues.

a221b9d

Fixed styling issues.

1ceab19

Fixed styling issues.

e142d8f

Fixed styling issues.

f0db1d8

evrii merged commit 1b9d704 into main Jan 20, 2023

evrii deleted the react-17 branch January 20, 2023 20:20

jmthibault79 mentioned this pull request Jan 26, 2023

[risk=low][RW-9451] rm UI duplicate FF enableCaptcha #7353

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[risk=low][RW-9199] Upgrade React to v17 #7167

[risk=low][RW-9199] Upgrade React to v17 #7167

jmthibault79 commented Nov 16, 2022 •

edited by evrii

Loading

jmthibault79 commented Nov 17, 2022

jmthibault79 commented Nov 17, 2022

jmthibault79 commented Nov 17, 2022

jmthibault79 commented Nov 17, 2022 •

edited

Loading

jmthibault79 commented Nov 21, 2022

jmthibault79 commented Nov 21, 2022

jmthibault79 Jan 13, 2023

evrii Jan 17, 2023

jmthibault79 left a comment

jmthibault79 Jan 18, 2023

dmohs left a comment

dmohs Jan 19, 2023

dmohs Jan 19, 2023

evrii Jan 20, 2023

dmohs Jan 19, 2023

-  await page.waitForFunction(() =>
-      document.querySelector('[role="button"][aria-label="Next"]').style.cursor !== 'not-allowed')
-  await page.click( '[role="button"][aria-label="Next"]')
+  await Promise.resolve('[role="button"][aria-label="Next"]').then(sel => {
+    page.waitForFunction(sel => document.querySelector(sel).style.cursor !== 'not-allowed', sel)
+    return sel
+  }).then(sel => page.click(sel))

[risk=low][RW-9199] Upgrade React to v17 #7167

[risk=low][RW-9199] Upgrade React to v17 #7167

Conversation

jmthibault79 commented Nov 16, 2022 • edited by evrii Loading

jmthibault79 commented Nov 17, 2022

jmthibault79 commented Nov 17, 2022

jmthibault79 commented Nov 17, 2022

jmthibault79 commented Nov 17, 2022 • edited Loading

jmthibault79 commented Nov 21, 2022

jmthibault79 commented Nov 21, 2022

jmthibault79 Jan 13, 2023

Choose a reason for hiding this comment

evrii Jan 17, 2023

Choose a reason for hiding this comment

jmthibault79 left a comment

Choose a reason for hiding this comment

jmthibault79 Jan 18, 2023

Choose a reason for hiding this comment

dmohs left a comment

Choose a reason for hiding this comment

dmohs Jan 19, 2023

Choose a reason for hiding this comment

dmohs Jan 19, 2023

Choose a reason for hiding this comment

evrii Jan 20, 2023

Choose a reason for hiding this comment

dmohs Jan 19, 2023

Choose a reason for hiding this comment

jmthibault79 commented Nov 16, 2022 •

edited by evrii

Loading

jmthibault79 commented Nov 17, 2022 •

edited

Loading