Skip to content

[PF3] Overlay component from react-bootstrap uses deprecated lifecycle method, causing warnings in React 16.9.x and will break in React 17 #3010

Closed
#3249
Closed
[PF3] Overlay component from react-bootstrap uses deprecated lifecycle method, causing warnings in React 16.9.x and will break in React 17#3010
#3249
Milestone
2019.10

Description

@mturley
mturley
opened on Sep 25, 2019

For our PF3 components, we depend on a legacy version of react-bootstrap (v0.32.4, which supports bootstrap v3: https://5c507d49471426000887a6a7--react-bootstrap.netlify.com/). They don't actively develop on that version anymore, since that library has moved on to bootstrap v4 and we can't upgrade it without breaking all of our PF3 styles.

That legacy version of react-bootstrap is now causing deprecation warnings in some of its components, such as Overlay:

Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://fb.me/react-async-component-lifecycle-hooks for details.

* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state
* Rename componentWillReceiveProps to UNSAFE_componentWillReceiveProps to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: Overlay

I'm opening this issue because this warning is showing up downstream in ManageIQ/manageiq-v2v#1037.

There is an issue open on react-bootstrap to release a fixed version of their legacy branch, which has been open for 22 days: react-bootstrap/react-bootstrap#4398

The deprecation warnings appear when using React 16.9.x, but the deprecated methods will be removed in React 17. Before any PF3-react consumers can upgrade to React 17, we will either need to see that fix released and upgrade our copy of react-bootstrap, or we'll need to copy any affected components in-house and fix them ourselves.

Activity

  1. mturley
    added
    bug 🐛
    on Sep 25, 2019
  2. tlabaj
    added
    PF3
    on Sep 27, 2019

  3. mturley commented on Sep 30, 2019

    @mturley
    mturley
    on Sep 30, 2019
    ContributorAuthor

    It looks like the react-bootstrap team is resolving this, so after their PR (react-bootstrap/react-bootstrap#4530) is merged we should be able to grab their next release to resolve this issue (at least in the components we use from react-bootstrap).

  4. mturley commented on Oct 21, 2019

    @mturley
    mturley
    on Oct 21, 2019
    ContributorAuthor

    It appears there is still some work ongoing in react-bootstrap to fix this, the issue to watch is here: react-bootstrap/react-bootstrap#4398

  5. martinpovolny commented on Oct 22, 2019

    @martinpovolny
    martinpovolny
    on Oct 22, 2019
    Contributor

    @tlabaj : 27 days since this issue affecting multiple products was opened. Any plans on fixing it?

  6. rachael-phillips
    added this to the Prioritized Backlog milestone on Oct 22, 2019

  7. mturley commented on Oct 22, 2019

    @mturley
    mturley
    on Oct 22, 2019
    ContributorAuthor

    @martinpovolny we are currently blocked by our dependency (react-bootstrap) where this issue originates, we're discussing our options now for either helping them move this along or working around it somehow. A workaround would require duplicating many of their components and fixing them in house, which I would prefer to avoid if possible.

  8. tlabaj commented on Oct 22, 2019

    @tlabaj
    tlabaj
    on Oct 22, 2019
    Contributor

    Just to clarify my earlier comment. By addressing this I mean, we plan to look at or options as a team to support this.

  9. mturley commented on Oct 23, 2019

    @mturley
    mturley
    on Oct 23, 2019
    ContributorAuthor

    One small update from react-bootstrap/react-bootstrap#4639:

    The code coverage has dropped for Overlay and OverlayTrigger, so if this gets merged eventually those files will need more tests. In the meantime I'm waiting for the maintainers to review this.
    In the meantime, you can silence all the other warnings by using the latest from the branch in your package.json: "react-bootstrap": "react-bootstrap/react-bootstrap#bs3-dev",

    I don't think we should switch to that branch yet, because it won't clear us to call this fixed and allow future upgrades to React 17 until they get the Overlay fixes merged. But once that PR is merged we can use that branch in package.json to fix the issue before react-bootstrap releases the fix to npm, if we want. We could also switch over sooner to silence most of the warnings, but since it wouldn't fix them all, I'm not sure if that's worth it.

  10. mturley commented on Oct 30, 2019

    @mturley
    mturley
    on Oct 30, 2019
    ContributorAuthor

    Good news @martinpovolny @tlabaj , the react-bootstrap devs released their fix for this in v0.33.0. I'll open a PR to upgrade our react-bootstrap to v0.33.0 in patternfly-react, and then once that is merged and released we can upgrade patternfly-react in manageiq to fix ManageIQ/manageiq-v2v#1037.

  11. mturley
    added 2 commits that reference this issue on Oct 30, 2019

    build(package.json): Upgrade react-bootstrap to fix React lifecycle m…

    5538a46

    build(package.json): Upgrade react-bootstrap to fix React lifecycle m…

    21589c6

  12. martinpovolny commented on Nov 4, 2019

    @martinpovolny
    martinpovolny
    on Nov 4, 2019
    Contributor

    @mturley : thank you for the update. Thanks for tracking this and caring.

    However:

    lucifer ruby-2.5.5 - [~/Projects/patternfly-react/packages/patternfly-3/patternfly-react] (master)$ grep -r componentWillReceiveProps .
./src/components/ClassificationBanner/ClassificationBanner.js:  componentWillReceiveProps(nextProps) {
./src/components/Wizard/Stories/mockWizardDeployContents.js:  componentWillReceiveProps(nextProps) {
./src/components/Pagination/Paginator.js:  componentWillReceiveProps(nextProps) {
./src/components/LoginPage/components/LoginCardComponents/LoginLanguagePicker.js:  componentWillReceiveProps(nextProps) {
./src/components/VerticalNav/VerticalNavItemHelper.js:  componentWillReceiveProps(newProps) {
./src/components/Slider/BootstrapSlider.js:  componentWillReceiveProps(nextProps) {
./src/components/ToastNotification/TimedToastNotification.js:  componentWillReceiveProps(nextProps) {
./src/components/ToastNotification/ToastNotificationList.js:  componentWillReceiveProps() {

    My original comment was meant mainly in regard to Patternfly's code.

    On that I got no response, or I am failing to see it.

  13. mturley commented on Nov 4, 2019

    @mturley
    mturley
    on Nov 4, 2019
    ContributorAuthor

    Hi @martinpovolny,

    What version of patternfly-react are you searching in? I'm sorry I didn't communicate better; those methods have also been fixed (renamed to their UNSAFE_ prefixed versions) 14 days ago in #3098. When I originally looked into this I noticed them but was going to resolve them all at once when I pulled in the react-bootstrap upgrades, but someone else beat me to it.

    If you pull the latest patternfly-react master now, you'll get the fixes for the instances you identified here. Once #3249 is also merged and both PR changes are released, you should be able to upgrade to the latest patternfly-react release and see no more warnings from either our in-house or our dependency components.

  14. mturley
    added a commit that references this issue on Nov 4, 2019

    build(package.json): Upgrade react-bootstrap to fix React lifecycle m…

    56b74d2
  15. tlabaj
    modified the milestones: Prioritized Backlog, 2019.10 on Nov 5, 2019
  16. tlabaj
    closed this as completedin #3249on Nov 5, 2019
  17. tlabaj
    added a commit that references this issue on Nov 5, 2019

    build(package.json): Upgrade react-bootstrap to fix React deprecated …

    0185bc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions