Upgrade to React 18 by koesie10 · Pull Request #2167 · github/vscode-codeql

koesie10 · 2023-03-13T15:40:08Z

This upgrades the webviews to use React 18. There should be no user-facing changes.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

The version of `@primer/react` we were using didn't have the correct types for the `ThemeProvider`. By upgrading these packages to their latest versions, all types are correct.

In the results view, `setState` was used to set some state, and then `loadResults` was called to set some other state. However, `setState` is asynchronous, so the `this.state` in `loadResults` was not the state that was set before the call. This commit fixes it by combining the two `setState` calls into one. The logic was hard to follow, so I'm not sure if this is the correct fix. The `shouldKeepOldResultsWhileRendering` state seems to be unnecessary now since everything is being done in `setState` call, but I'm not sure about that.

koesie10 · 2023-03-14T09:48:13Z

Thanks for the review @charisk! I've had to make some other changes to make it fully work with React 18. The results view wasn't working due to how React is now batching state updates, so there were quite some changes in how the results view now receives some messages.

@aeisenberg Would you be able to review those changes as well? It's mostly about 36eb2cd. I don't believe there are any major behaviour changes, but we're not using shouldKeepOldResultsWhileRendering anymore since we're loading the results in the same setState call as where that gets set.

elenatanasoiu · 2023-03-14T14:46:06Z


  private updateStateWithNewResultsInfo(resultsInfo: ResultsInfo): void {
    this.setState((prevState) => {
-      const stateWithDisplayedResults = (


Could we write tests for the behaviour before we made this change and then see if the same behaviour happens afterwards? That would mean we wouldn't need to guess if we've completely got it right.

I think writing tests for this is really hard and the previous behaviour will not be observable in tests. Adding tests would probably take at least a day, and since we'll be rewriting these views soon anyway, I don't really see a lot of added value in it.

Is it hard because we would have to check visual components or because it's a timing issue?

It's a timing issue, there's no real way to check the state in between the two setState calls, so we'd never get into the state that we'd want to test. We'd want to check what the view looks like in between these two calls but they are called sequentially without any waiting, so there's no way to actually test that state.

I'm gonna defer to @aeisenberg on this one. I would ask if we can get rid of shouldKeepOldResultsWhileRendering entirely if we're combining calls.

Looks like Andrew is on holiday so you might have to wait a bit with this one.

Just wanted to say that I wasn't expecting us to do a complete re-write of this view soon as the changes we'd like to make are mostly cosmetic. But I can be convinced otherwise. Also I don't mean that to say that we have to spend a day writing tests - the component style does look a bit old so likely not worth the investment.

This method is overly complex and a simplification would be really nice. To be safe when we are doing a refactoring like this, it would be nice to add some pure regression unit tests to ensure that all the different cases are handled identically before and after.

Although nice, it's not necessary to spend the time to create more complex tests that check the view between the two setState calls. All that I think is necessary is that we ensure the method behaves the same before and after the refactoring. It's a complicated method, but at least the complexity is contained.

I've created a really simple regression test in #2255. This does not test the loading state, but checks that we can at least render a SARIF file.

elenatanasoiu · 2023-03-14T14:48:12Z

Do we get any extra warnings now that we've upgraded to React 18? I'd be surprised if we have a clean upgrade.

This removes the comment about the `viewLoaded` callback since it refers to upgrade instructions.

koesie10 · 2023-03-14T15:16:13Z

Do we get any extra warnings now that we've upgraded to React 18? I'd be surprised if we have a clean upgrade.

We don't have any extra warnings since we're actually not using that many React features. What kind of warnings would you have expected to see?

elenatanasoiu · 2023-03-14T15:19:06Z

I was expecting to see warnings when we run npm run build / npm run watch / npm install, during CI or when running tests locally. But if those are clean, that's great. I also see you're enabling strict mode in a different PR so maybe that will show more warnings in the future.

charisk · 2023-03-16T08:43:57Z

Thanks for the review @charisk! I've had to make some other changes to make it fully work with React 18. The results view wasn't working due to how React is now batching state updates, so there were quite some changes in how the results view now receives some messages.

Thanks for the poke! The changes look okay to me, but as you say the logic is quite hard to follow. I've also tested them a bit manually and things seem ok to me.

elenatanasoiu · 2023-03-16T09:28:48Z

Thanks for testing @charisk! I wasn't sure how to go about checking this. If we know what to expect in the behaviour change, I don't want to hold this up.

charisk · 2023-03-16T09:30:24Z

Thanks for testing @charisk! I wasn't sure how to go about checking this. If we know what to expect in the behaviour change, I don't want to hold this up.

Sorry I wasn't clear - I didn't necessarily test the specific use case. I just poked the view a bit.

aeisenberg · 2023-03-30T17:24:04Z

+            isExpectingResultsUpdate: prevState.isExpectingResultsUpdate,
+            nextResultsInfo: resultsInfo,
+          };
+        }


Are you missing a case here? I don't see line 191 from the original handled.

return stateWithDisplayedResults(prevState.displayedResults);

This is some gnarly updating code. Just eyeballing things, is this a better simplification:

this.setState((prevState) => { let displayedResults: ResultsState | undefined = undefined; if (!prevState.isExpectingResultsUpdate && resultsInfo === null) { // No results to display displayedResults = { resultsInfo: null, results: null, errorMessage: "No results to display", }; } else if (!resultsInfo?.shouldKeepOldResultsWhileRendering) { // Display loading message displayedResults = { resultsInfo: null, results: null, errorMessage: "Loading results…", }; } else { displayedResults = prevState.displayedResults; } return { displayedResults, isExpectingResultsUpdate: prevState.isExpectingResultsUpdate, nextResultsInfo: resultsInfo, }; });

Unit tests would be great for making sure the semantics of updateStateWithNewResultsInfo haven't changed.

In this case, the missing case is correct. In the previous code, after the setState returned with stateWithDisplayedResults(prevState.displayedResults), this.loadResults() would be called immediately. The only reason for storing the nextResultsInfo in the state is so that loadResults can use it. However, with concurrent updates the state seen in loadResults would not yet be the state set in this setState call. Therefore, I've combined this setState and the loadResults method. Therefore, this case won't happen anymore.

Thanks for the suggestion for the simplification. Since we don't need the third case, I couldn't use it as-is. However, I have split the flows differently so there's only 1 level of nesting rather than 2.

aeisenberg · 2023-03-30T17:33:04Z


  private updateStateWithNewResultsInfo(resultsInfo: ResultsInfo): void {
    this.setState((prevState) => {
-      const stateWithDisplayedResults = (


This method is overly complex and a simplification would be really nice. To be safe when we are doing a refactoring like this, it would be nice to add some pure regression unit tests to ensure that all the different cases are handled identically before and after.

Although nice, it's not necessary to spend the time to create more complex tests that check the view between the two setState calls. All that I think is necessary is that we ensure the method behaves the same before and after the refactoring. It's a complicated method, but at least the complexity is contained.

aeisenberg

This looks good now, but can you rebase on #2255 so we can be sure that the behaviour remains consistent?

Upgrade to React 18

e949b96

koesie10 added the secexp label Mar 13, 2023

koesie10 requested a review from a team March 13, 2023 15:40

charisk approved these changes Mar 14, 2023

View reviewed changes

koesie10 added 3 commits March 14, 2023 10:02

Upgrade @primer packages

4a29509

The version of `@primer/react` we were using didn't have the correct types for the `ThemeProvider`. By upgrading these packages to their latest versions, all types are correct.

Mock CSS.supports in view tests

605315c

koesie10 marked this pull request as ready for review March 14, 2023 09:45

koesie10 requested a review from a team as a code owner March 14, 2023 09:45

elenatanasoiu reviewed Mar 14, 2023

View reviewed changes

koesie10 added 2 commits March 14, 2023 16:10

Extract variable for React root

b888ca0

Remove viewLoaded callback comment

db96744

This removes the comment about the `viewLoaded` callback since it refers to upgrade instructions.

elenatanasoiu reviewed Mar 14, 2023

View reviewed changes

Comment thread extensions/ql-vscode/src/view/jest.setup.ts

elenatanasoiu approved these changes Mar 16, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into koesie10/react-18

d4464c8

aeisenberg reviewed Mar 30, 2023

View reviewed changes

koesie10 added 3 commits March 31, 2023 13:52

Better error message and check for results info

918fb75

Use getErrorMessage

b4b0208

Reduce nesting in if-statement

cb2b4f4

aeisenberg reviewed Mar 31, 2023

View reviewed changes

Merge branch 'main' into koesie10/react-18

e997dab

koesie10 merged commit 980e27a into main Apr 3, 2023

koesie10 deleted the koesie10/react-18 branch April 3, 2023 08:47

Conversation

koesie10 commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

koesie10 commented Mar 14, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charisk Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elenatanasoiu commented Mar 14, 2023

Uh oh!

koesie10 commented Mar 14, 2023

Uh oh!

elenatanasoiu commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

charisk commented Mar 16, 2023

Uh oh!

elenatanasoiu commented Mar 16, 2023

Uh oh!

charisk commented Mar 16, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

koesie10 commented Mar 13, 2023 •

edited

Loading

charisk Mar 16, 2023 •

edited

Loading

elenatanasoiu commented Mar 14, 2023 •

edited

Loading