Skip to content
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

Jump to bottom

Update to React 18.3.1, Storybook 8.2.9, and vitest 2.0.5 #2230

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Update to React 18.3.1, Storybook 8.2.9, and vitest 2.0.5 #2230

merged 2 commits into from
Aug 27, 2024

Conversation

joaquimrocha
Copy link
Collaborator

I tried to update the deps to work with react 18 and remove the mui/styles compatibility package.
Headlamp seems to work fine but the storybook tests are failing (snapshots not rendering anything in DocumentFragment).

fixes #1554

cc/ @sniok , @vyncent-t

@vyncent-t
Copy link
Contributor

I think this may be due to the new react docgen that the new storybook uses

image

when running npx storybook@latest automigrate to use the newest storybook from this PR it will ask if we want to revert to typescript

@sniok sniok force-pushed the update-to-react-18 branch 3 times, most recently from 956be60 to 6ff3dca Compare August 8, 2024 11:47
@sniok
Copy link
Contributor

sniok commented Aug 8, 2024 •
edited
Loading

storybook tests are now working but still need to fix some type errors, React 18 changed how children are defined

@sniok sniok force-pushed the update-to-react-18 branch 3 times, most recently from c8fd248 to 5fa8555 Compare August 9, 2024 10:10
@joaquimrocha joaquimrocha marked this pull request as ready for review August 9, 2024 10:26
@joaquimrocha joaquimrocha changed the title WIP: Update to React 18 Update to React 18 Aug 9, 2024
@joaquimrocha joaquimrocha marked this pull request as draft August 9, 2024 11:04
@joaquimrocha
Copy link
Collaborator Author

I have renamed my commit and pushed again, but in my machine, the tests aren't passing. It tries to delete the View YAML button from the snapshots...

@sniok sniok force-pushed the update-to-react-18 branch from 7a29b32 to 14cd60a Compare August 9, 2024 11:27
@sniok sniok force-pushed the update-to-react-18 branch 12 times, most recently from ae850fa to 07132e4 Compare August 19, 2024 11:46
@sniok sniok marked this pull request as ready for review August 19, 2024 11:46
@sniok sniok mentioned this pull request Aug 19, 2024
Merged
3 tasks
@sniok sniok force-pushed the update-to-react-18 branch from 07132e4 to 527abd8 Compare August 20, 2024 07:52
joaquimrocha
joaquimrocha commented Aug 21, 2024
View reviewed changes
Copy link
Collaborator Author

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

@sniok , I just noticed a couple of things that should be addressed, and a nit.

frontend/.storybook/preview.tsx Outdated Show resolved Hide resolved
frontend/.storybook/baseMocks.ts Show resolved Hide resolved
frontend/src/components/common/ObjectEventList.tsx Outdated Show resolved Hide resolved
frontend/src/components/horizontalPodAutoscaler/HPAList.stories.tsx Outdated Show resolved Hide resolved
@sniok sniok force-pushed the update-to-react-18 branch 3 times, most recently from 0bd4146 to 859c294 Compare August 26, 2024 07:58
illume
illume reviewed Aug 26, 2024
View reviewed changes
frontend/src/index.tsx Outdated Show resolved Hide resolved
illume
illume requested changes Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@illume illume left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tests pass for me locally now.

Smaller separate commits/PRs would be good for some of these changes. Many of them could be brought in separate PRs before an update to react 18 is done. This would make the PR smaller so the github UI is happy and usable. It will also mean there are commit messages explaining some of the changes.

These things that are removed should be called out in the PR description:

Can you please add a "Testing done" section to the PR description? I expect we will need to manually test the whole frontend.

@sniok sniok force-pushed the update-to-react-18 branch 3 times, most recently from 252b4a2 to eb83de6 Compare August 26, 2024 13:50
illume
illume reviewed Aug 27, 2024
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
@sniok sniok force-pushed the update-to-react-18 branch from eb83de6 to 4938dfe Compare August 27, 2024 08:42
@sniok sniok mentioned this pull request Aug 27, 2024
Open
This was referenced Aug 27, 2024
Merged
Merged
Merged
@sniok sniok force-pushed the update-to-react-18 branch from 4938dfe to 6fe2478 Compare August 27, 2024 09:47
@illume illume changed the title Update to React 18 Update to React 18.3.1, Storybook 8.2.9, and vitest 2.0.5 Aug 27, 2024
@illume
Copy link
Collaborator

illume commented Aug 27, 2024

@joaquimrocha can you please test this locally now?

We understand the Notifications test was sometimes failing. We decided to remove the Notifications test because that code isn't being used at the moment anyway.

I'm giving this a final test now.

@sniok sniok force-pushed the update-to-react-18 branch 2 times, most recently from bdcb75e to 1b437ee Compare August 27, 2024 15:24
joaquimrocha and others added 2 commits August 27, 2024 17:33
@joaquimrocha @sniok
frontend: Update React to version 18
c6990a1 
Co-authored-by: Oleksandr Dubenko <oldubenko@microsoft.com>

Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
@sniok
frontend: Upgrade storybook setup
2f91c6e 
This is a pretty big commit that upgrades storybook version and also
changes the way we mock backend in stories.

Previosly stories would override methods in particular resources
Like Pod.useList = () => mockData. But this wasn't playing nicely with
vite and wasn't reliable as we were modifying global values during
runtime.

Instead all of that mocking is happening in the mock service worker
(msw). Which intersepts network requests and is a reliable way of
mocking.

In addition to that, the storybook.test.tsx also had some changes to run
the stories in a way the documentation recommends. This also changed the
output of the snapshots slightly, which required regeneration of all
snapshots.

Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
@sniok sniok force-pushed the update-to-react-18 branch from 1b437ee to 2f91c6e Compare August 27, 2024 15:37
@joaquimrocha
Copy link
Collaborator Author

The home was also failing, and since the components were used elsewhere, @sniok removed it too. Now this PR is succeeding for me in the several attempts I tried. @illume , if it LGTY, let's merge it and move forward.

@illume
Copy link
Collaborator

illume commented Aug 27, 2024

The test-plugins job hanging forever is not related to this PR, it's happening on other PRs too.

illume
illume approved these changes Aug 27, 2024
View reviewed changes
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

Thanks 🎉🎈

@illume illume merged commit 8c529cc into main Aug 27, 2024
17 of 18 checks passed
@illume illume deleted the update-to-react-18 branch August 27, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sniok sniok sniok left review comments

@illume illume illume approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to React 18
4 participants
@joaquimrocha @vyncent-t @sniok @illume