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

[LOOM-1609]: React 18 migration #3585

Merged
merged 15 commits into from
Aug 26, 2024
Merged

[LOOM-1609]: React 18 migration #3585

merged 15 commits into from
Aug 26, 2024

Conversation

olliecurtis
Copy link
Member

Remember to include the following changes:

  • Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Accessibility tests
    • The following checks were performed:
      • Ability to navigate using a keyboard only
      • Zoom functionality (Deque University explanation):
        • The page SHOULD be functional AND readable when only the text is magnified to 200% of its initial size
        • Pages must reflow as zoom increases up to 400% so that content continues to be presented in only one column i.e. Content MUST NOT require scrolling in two directions (both vertically and horizontally)
      • Ability to navigate using a screen reader only
  • Storybook examples created/updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@olliecurtis olliecurtis changed the title [ARGG-1201]: React 18 investigation [WIP][ARGG-1201]: React 18 investigation Aug 13, 2024
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

1 similar comment
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@olliecurtis olliecurtis force-pushed the ARGG-1201-React-18-investigation branch from aec50ff to 264ded0 Compare August 14, 2024 15:56
@olliecurtis olliecurtis mentioned this pull request Aug 14, 2024
Closed
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

1 similar comment
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@olliecurtis olliecurtis force-pushed the ARGG-1201-React-18-investigation branch from ef03a70 to 0416b2b Compare August 16, 2024 15:36
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
dangerfile.ts
@@ -149,10 +149,11 @@ const reactRecogniseProp = ["React does not recognize"]
const invalidTags = ["The tag <rect>", "The tag <g>", "The tag <text>"]
const passingTests = ["✓"]
const unknownEventHandler = ["Unknown event handler"]
const propType = ["Failed prop type"]
const propType = ["Failed prop type", "Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.", 'A props object containing a "key" prop is being spread into JSX:']
Copy link
Member Author

Choose a reason for hiding this comment

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

The first new addition has been added because we will need to ignore this until we have full conversion of components to Typescript.

For the second it shall be addressed in this PR or a separate one as this is new

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
dangerfile.ts
const componentWillReceiveProps = ["componentWillReceiveProps"]
const invalidCSSProperties = ["is an invalid value for the .* css style property."]
const invalidProps = ["for a non-boolean attribute", "Invalid ARIA attribute"]
const actTests = ["inside a test was not wrapped in act(...)"]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a newly introduced warning, tests still pass and there is a few conversations open for this

https://github.com/reactwg/react-18/discussions/102

As its not a blocker adding it here now and we should revisit in the future

@olliecurtis olliecurtis force-pushed the ARGG-1201-React-18-investigation branch from d645a16 to 30070cb Compare August 19, 2024 16:31
@Skyscanner Skyscanner deleted a comment from github-actions bot Aug 19, 2024
@olliecurtis olliecurtis added the minor Non breaking change label Aug 19, 2024
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 19, 2024 •
edited
Loading

Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.tsx) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 769c8ad

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
dangerfile.ts
// TODO: Address tests being wrapped in act
const actTests = ["inside a test was not wrapped in act(...)"]
// TODO: Convert components that use CSSTransition to functional components and allow for using refs
const findDOMNode = ["findDOMNode is deprecated and will be removed in the next major release."];
Copy link
Member Author

Choose a reason for hiding this comment

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

This is coming from the underlying react-transition-group library

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
packages/bpk-component-breakpoint/src/useMediaQuery-test.tsx

expect(view.result.all[0]).toBe(true);

expect(view.result.current).toBe(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

The structure of renderHook returns a different object where the value is located

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
packages/bpk-component-carousel/src/utils.test.tsx Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to check where or what this has come from 🤔

I think it might have been an error thrown that there was no tests for this

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
packages/bpk-component-infinite-scroll/src/__snapshots__/withInfiniteScroll-test.js.snap
Comment on lines -34 to -50
>
<div>
Element 0
</div>
<div>
Element 1
</div>
<div>
Element 2
</div>
<div>
Element 3
</div>
<div>
Element 4
</div>
</div>
Copy link
Member Author

Choose a reason for hiding this comment

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

🤔

This is probably also why snapshot testing is risky as a form of testing

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
packages/bpk-theming/src/BpkThemeProvider.js
themeAttributes,
...rest
} = this.props;
const BpkThemeProvider = (props) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

We change this to be a functional component as the getChildContext is deprecated and will be removed in a future version.

olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
packages/package.json Outdated Show resolved Hide resolved
olliecurtis
olliecurtis commented Aug 19, 2024
View reviewed changes
scripts/jest/setup.js
@@ -19,6 +19,8 @@
import 'jest-axe/extend-expect';
import '@testing-library/jest-dom';
import 'raf/polyfill';
import { TextEncoder } from 'util'
Copy link
Member Author

Choose a reason for hiding this comment

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

This mock is required as part of the upgrade as in a newer version of jsdom this is not exposed/available to the environment so causes tests to fail due to underlying global not being available

@olliecurtis olliecurtis marked this pull request as ready for review August 20, 2024 13:11
@olliecurtis olliecurtis changed the title [WIP][ARGG-1201]: React 18 investigation [ARGG-1201]: React 18 investigation Aug 20, 2024
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@olliecurtis olliecurtis changed the title [ARGG-1201]: React 18 investigation [LOOM-1609]: React 18 migration Aug 20, 2024
@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

Visit https://backpack.github.io/storybook-prs/3585 to see this build running in a browser.

@olliecurtis olliecurtis merged commit 01428ca into main Aug 26, 2024
9 checks passed
@olliecurtis olliecurtis deleted the ARGG-1201-React-18-investigation branch August 26, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xalechez xalechez xalechez approved these changes

Assignees
No one assigned
Labels
minor Non breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@olliecurtis @xalechez