C2LC-575: Update react-scripts to version 5 #317

sbates-idrc · 2021-12-21T18:35:41Z

See: https://issues.fluidproject.org/browse/C2LC-575

* With the update, ESLint started generating 'no-use-before-define' warnings when a class method has a Flow annotation that states that the method returns an instance of itself. We use this pattern in the CharacterState and ProgramSequence classes. This commit adds comments to suppress these ESLint warnings. * Update the Netlify build to Node 14 (react-scripts version 5 no longer supports Node 12) * Update the GitHub Actions CI build to Node 14 and 16

With the update to react-scripts 5.0.0, flow-coverage-report is failing with an error similar to this issue filed against flow-coverage-report on 2021-10-21: rpl/flow-coverage-report#208

the-t-in-rtf · 2022-01-11T09:12:55Z

We should discuss in today's meeting, I think we can keep the flow coverage reporting if we simply switch to using flow-coverage-report via npx. In testing with your branch locally, it looks like the coverage reporting still works that way. That seems the least effort and we keep the value of the report.

the-t-in-rtf · 2022-01-11T09:22:42Z

I see that the CI checks already used npx. Was there an example of a build that failed or output that was incorrect? In local testing with node@14, all function as they did before.

sbates-idrc · 2022-01-11T20:26:18Z

Thanks Tony.

This is the failure that I saw in CI:

https://github.com/sbates-idrc/c2lc-coding-environment/runs/4597242223?check_suite_focus=true

And the issue filed against flow-coverage-report showing a similar failure:

rpl/flow-coverage-report#208

the-t-in-rtf · 2022-01-12T08:53:55Z

I compared the output of npm audit when run against develop-1.2 and this branch. This pull resolves 9 moderate severity issues, 10 high, and one critical, leaving us with 2 low severity and 8 medium.

It looks like five more medium severity vulnerabilities would be resolved if we updated node-sass, I filed C2LC-578 to cover that work. We still have some vulnerabilities brought in by react-scripts, these will be whitelisted when we work on C2LC-553.

the-t-in-rtf · 2022-01-12T09:04:54Z

I compared the packed content sizes in the deployed develop-1.2 preview to those in this build, there were no red flags I could find. The updated version ends up producing a few kilobytes more content, but overall the speed seems fine.

the-t-in-rtf · 2022-01-12T09:10:32Z

package.json

@@ -7,18 +7,17 @@
        "classnames": "2.3.1",
        "enzyme": "3.11.0",
        "enzyme-adapter-react-16": "1.15.6",
-        "node-sass": "4.14.1",
+        "node-sass": "7.0.0",


I didn't see that you had updated this already when I commented earlier and filed C2LC-578. If we're already updating, then we should go ahead and update to 7.0.1.

Done. Thanks.

the-t-in-rtf · 2022-01-12T09:11:57Z

src/CharacterState.js

@@ -60,6 +60,7 @@ export default class CharacterState {
        return true;
    }

+    // eslint-disable-next-line no-use-before-define


Are there any circumstances in which this check is helpful to us? If not, maybe we should disable the rule rather than adding all these comments.

That's a good question. Disabling the rule, would certainly make the code cleaner by not requiring these comments.

I think the rule offers some value but I think not a huge amount. I'll disable the rule and remove the comments. We can always reconsider. But I think minimizing the number of ESLint-specific comments is worthwhile.

I've disabled the "no-use-before-define" rule

the-t-in-rtf · 2022-01-12T09:19:30Z

I left a couple of minor comments, otherwise this looks good.

sbates-idrc added 4 commits December 21, 2021 11:15

C2LC-575: Disable flow-coverage-report

7f24b42

With the update to react-scripts 5.0.0, flow-coverage-report is failing with an error similar to this issue filed against flow-coverage-report on 2021-10-21: rpl/flow-coverage-report#208

C2LC-575: Update node-sass to 7.0.0

8ab5003

C2LC-575: Remove flow-coverage-report

ba470c9

the-t-in-rtf reviewed Jan 12, 2022

View reviewed changes

sbates-idrc added 2 commits January 12, 2022 09:17

C2LC-575: Update node-sass to 7.0.1

eab585e

C2LC-575: Disable ESLint "no-use-before-define"

fbd2b4e

sbates-idrc merged commit 5c0cfa5 into codelearncreate:develop-1.2 Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C2LC-575: Update react-scripts to version 5 #317

C2LC-575: Update react-scripts to version 5 #317

sbates-idrc commented Dec 21, 2021

the-t-in-rtf commented Jan 11, 2022

the-t-in-rtf commented Jan 11, 2022

sbates-idrc commented Jan 11, 2022

the-t-in-rtf commented Jan 12, 2022 •

edited

Loading

the-t-in-rtf commented Jan 12, 2022 •

edited

Loading

the-t-in-rtf Jan 12, 2022

sbates-idrc Jan 12, 2022

the-t-in-rtf Jan 12, 2022

sbates-idrc Jan 12, 2022 •

edited

Loading

sbates-idrc Jan 17, 2022

the-t-in-rtf commented Jan 12, 2022

C2LC-575: Update react-scripts to version 5 #317

C2LC-575: Update react-scripts to version 5 #317

Conversation

sbates-idrc commented Dec 21, 2021

the-t-in-rtf commented Jan 11, 2022

the-t-in-rtf commented Jan 11, 2022

sbates-idrc commented Jan 11, 2022

the-t-in-rtf commented Jan 12, 2022 • edited Loading

the-t-in-rtf commented Jan 12, 2022 • edited Loading

the-t-in-rtf Jan 12, 2022

Choose a reason for hiding this comment

sbates-idrc Jan 12, 2022

Choose a reason for hiding this comment

the-t-in-rtf Jan 12, 2022

Choose a reason for hiding this comment

sbates-idrc Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

sbates-idrc Jan 17, 2022

Choose a reason for hiding this comment

the-t-in-rtf commented Jan 12, 2022

the-t-in-rtf commented Jan 12, 2022 •

edited

Loading

the-t-in-rtf commented Jan 12, 2022 •

edited

Loading

sbates-idrc Jan 12, 2022 •

edited

Loading