Skip to content

Update React 16.4 (mixed versions) to React 16.8#1541

Merged
dgutride merged 20 commits into
masterfrom
chore/react-16.8
Mar 13, 2019
Merged

Update React 16.4 (mixed versions) to React 16.8#1541
dgutride merged 20 commits into
masterfrom
chore/react-16.8

Conversation

@redallen
Copy link
Copy Markdown
Contributor

@redallen redallen commented Mar 11, 2019 •
edited
Loading

What:

  • Upgrade and lock to React 16.8 across all projects.
  • Upgrade and lock to Enzyme 3.9.0.
  • Remove broken commitlint package since we no longer enforce semantic commits
  • Update some test selectors for new Enzyme-react adapter
  • Fix Gatsby by making it ignore *.d.ts files
  • Huge yarn.lock bump
  • Fix Avatar in Gatsby docs

Additional issues: Pf4 doc build is broken. Working with @jschuler to fix. Fixed

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 11, 2019 •
edited
Loading

Codecov Report

Merging #1541 into master will increase coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1541     +/-   ##
=========================================
+ Coverage   83.62%   83.82%   +0.2%     
=========================================
  Files         552      556      +4     
  Lines        5751     5856    +105     
  Branches       12       12             
=========================================
+ Hits         4809     4909    +100     
- Misses        940      945      +5     
  Partials        2        2
Flag Coverage Δ
#patternfly3 84.89% <ø> (ø) ⬆️
#patternfly4 81.4% <ø> (ø) ⬆️
#patternflymisc 95.65% <ø> (?)
Impacted Files Coverage Δ
test.env.js 100% <ø> (ø) ⬆️
...y-3/patternfly-react/src/common/closestPolyfill.js 81.81% <0%> (-9.1%) ⬇️
packages/react-icons/src/common.js 100% <0%> (ø)
packages/react-icons/src/createIcon.js 100% <0%> (ø)
packages/react-codemods/transforms/pf3-pf4.js 94.04% <0%> (ø)
packages/react-codemods/componentConfig.js 100% <0%> (ø)
...ernfly-3/patternfly-react/src/common/controlled.js 75% <0%> (+5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4edacf0...db56c96. Read the comment docs.

@redallen redallen mentioned this pull request Mar 11, 2019
Merged
@redallen redallen self-assigned this Mar 11, 2019
@dgutride
Copy link
Copy Markdown
Member

dgutride commented Mar 11, 2019

@redallen - we'd like to get this tested with two other products to ensure there are no breaking changes. Also - what is left to get the do not merge label removed?

@redallen
Copy link
Copy Markdown
Contributor Author

redallen commented Mar 11, 2019

All that's left is getting gatsby doc generation working. Weird typescript-related errors are occurring.

@redallen redallen mentioned this pull request Mar 11, 2019
Merged
@redallen redallen changed the title [WIP] Update React 16.4 (mixed versions) to React 16.8 Update React 16.4 (mixed versions) to React 16.8 Mar 11, 2019
@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Mar 11, 2019

PatternFly-React preview: https://1541-pr-patternfly-react-patternfly.surge.sh

@redallen redallen requested review from dlabaj and dmiller9911 March 11, 2019 21:21
@dgutride dgutride removed the request for review from dmiller9911 March 12, 2019 13:00
dgutride
dgutride suggested changes Mar 12, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dgutride dgutride left a comment

Choose a reason for hiding this comment

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

Generally - I think this looks good, mostly snapshot updates. In the future, it would be good to break these into much smaller PR's to ease reviewing. For instance - could we have only updated PF4 at this time - should we make sure any react dependencies are decoupled between pf3 and pf4 to allow them to move independently?

Comment thread package.json
@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Mar 12, 2019

PatternFly-React preview: https://1541-pr-patternfly-react-patternfly.surge.sh

dgutride
dgutride previously approved these changes Mar 12, 2019
View reviewed changes
mturley
mturley reviewed Mar 12, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/components/ChipGroup/__snapshots__/Chip.test.js.snap
/>
>
<Component />
</GenerateId>
Copy link
Copy Markdown
Collaborator

@mturley mturley Mar 12, 2019

Choose a reason for hiding this comment

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

This is a pre-existing problem, but we should probably revisit these snapshot tests later. A snapshot of a shallow render of something wrapped by GenerateId or ContextConsumer isn't accomplishing anything.

mturley
mturley reviewed Mar 12, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-docs/src/components/propsTableTs/propsTableTs.js Outdated
</Table>
</Section>
);
// {props.map(prop => (
Copy link
Copy Markdown
Collaborator

@mturley mturley Mar 12, 2019

Choose a reason for hiding this comment

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

We should avoid committing commented-out code, right?

Also, when trying to check the effects of removing this, I went to look at the props table for Avatar and on https://1541-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/avatar I'm getting a blank page with console errors:

TypeError: Cannot read property 'name' of null
    at getEnumValue (propsTable.js:68)
    at propsTable.js:53
    at Array.map (<anonymous>)
    at PropsTable (propsTable.js:49)
    at hg (react-dom.production.min.js:150)
    at Tg (react-dom.production.min.js:178)
    at bi (react-dom.production.min.js:232)
    at ci (react-dom.production.min.js:233)
    at Di (react-dom.production.min.js:249)
    at Yh (react-dom.production.min.js:248)
qh @ react-dom.production.min.js:198
react-dom.production.min.js:248 Uncaught TypeError: Cannot read property 'name' of null
    at getEnumValue (propsTable.js:68)
    at propsTable.js:53
    at Array.map (<anonymous>)
    at PropsTable (propsTable.js:49)
    at hg (react-dom.production.min.js:150)
    at Tg (react-dom.production.min.js:178)
    at bi (react-dom.production.min.js:232)
    at ci (react-dom.production.min.js:233)
    at Di (react-dom.production.min.js:249)
    at Yh (react-dom.production.min.js:248)

Copy link
Copy Markdown
Contributor Author

@redallen redallen Mar 12, 2019

Choose a reason for hiding this comment

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

Yeah, that's a problem. @jschuler has found a way around it in #1539. For now I'll find a way to disable more functionality for the time being.

Copy link
Copy Markdown
Contributor Author

@redallen redallen Mar 12, 2019

Choose a reason for hiding this comment

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

Fixed in latest commit by changing a little bit of Gatsby logic. New preview should deploy shortly.

Copy link
Copy Markdown
Contributor Author

@redallen redallen Mar 12, 2019

Choose a reason for hiding this comment

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

New preview doesn't seem to have console errors for Avatar: https://1541-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/avatar

mturley
mturley reviewed Mar 12, 2019
View reviewed changes
Comment thread .gitignore Outdated
mturley
mturley reviewed Mar 12, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/scripts/convertComponentHelpers.js Outdated
mturley
mturley reviewed Mar 12, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

This is great work @redallen, I especially love the scripts for converting components to TS! Just left a couple of minor comments.

dlabaj
dlabaj approved these changes Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

mturley
mturley approved these changes Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
Can't wait to build some stuff with Hooks.

@dgutride dgutride merged commit 86a4715 into master Mar 13, 2019
@redallen redallen deleted the chore/react-16.8 branch March 18, 2019 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlabaj dlabaj dlabaj approved these changes

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

@tlabaj tlabaj Awaiting requested review from tlabaj

+2 more reviewers

@mturley mturley mturley approved these changes

@dgutride dgutride dgutride left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@redallen redallen

Labels

Infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@redallen @codecov-io @dgutride @patternfly-build @mturley @dlabaj