chore: update build job to include multiple major react versions #7125
Conversation
davidmenendez
requested review from
makafsal and
anamikaanu96
and removed request for
a team
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7125 +/- ##
==========================================
+ Coverage 84.48% 92.14% +7.66%
==========================================
Files 425 26 -399
Lines 17247 4151 -13096
Branches 4575 263 -4312
==========================================
- Hits 14571 3825 -10746
+ Misses 2676 326 -2350
🚀 New features to boost your workflow:
|
|
update: the action appears to be working as intended, but unfortunately the tests are failing. i'm currently investigating getting the tests to pass in v17 |
|
code coverage dropped, but it's necessary at the moment until we can fix a lot of these flaky tests. |
|
@matthewgallo i'm going to add a followup issue to address this. but basically there's some issue with async functionality between the major test versions. rather then try to hunt down the problematic code on this PR i decided to just skip it for now. |
...mponents/FilterPanel/FilterPanelCheckboxWithOverflow/FilterPanelCheckboxWithOverflow.test.js
Outdated
Show resolved
Hide resolved
| title, | ||
| tooltipAlign, | ||
| ...rest | ||
| } = props; |
There was a problem hiding this comment.
More of a question, I've seen this change across a handful of components, how come you've moved the prop destructuring to inside the component?
There was a problem hiding this comment.
i find this pattern to be useful because you can easily log all the props before separating them. it also declutters the function signature a bit.
packages/ibm-products/src/components/CoachmarkStack/CoachmarkStackHome.tsx
Outdated
Show resolved
Hide resolved
packages/ibm-products/src/components/CoachmarkStack/CoachmarkStackHome.tsx
Outdated
Show resolved
Hide resolved
| - name: Add React version ${{ matrix.react }} | ||
| run: | | ||
| yarn add react@${{ matrix.react }} react-dom@${{ matrix.react }} | ||
| yarn workspace @carbon/ibm-cloud-cognitive-core add react@${{ matrix.react }} react-dom@${{ matrix.react }} -D |
There was a problem hiding this comment.
| yarn workspace @carbon/ibm-cloud-cognitive-core add react@${{ matrix.react }} react-dom@${{ matrix.react }} -D | |
| yarn workspace @carbon/ibm-products add react@${{ matrix.react }} react-dom@${{ matrix.react }} -D |
| yarn workspace @carbon/ibm-cloud-cognitive-core add @carbon/react@^1.28.0 -D | ||
| yarn workspace @carbon/ibm-products add @carbon/react@^1.28.0 -P |
There was a problem hiding this comment.
Do you need both of these lines? Probably just the second, right? packages/core isn't really used for anything now.
Also, how come you're installing 1.28.0?
There was a problem hiding this comment.
yeah good call now that core has been decoupled.
1.28 if i remember correctly i think this version didn't include a bug that was added in a subsequent 1.x version. this PR has been sitting around for a while so i wish i could remember the specifics of why up top, but i remember there was a specific reason
There was a problem hiding this comment.
hmm looks like core still needs to be included or something breaks. this might need to be looked at separately
|
its been 4 months since this PR was opened and now that we're on the cusp of react 19 support i don't think it makes sense to keep this going. i will open up a follow up issue to investigate CI testing for 18 + 19. based on what i've seen with the latest major version of jest it may prove to be just as tricky to implement, but we will see! |
Closes #7044 & #7137
adds a new job for testing other major versions of React. since we are currently on v18 i didn't think it made sense to include
18in the matrix, but if we wanted to be extra safe we could also include it[17, 18]