search

bencripps / react-redux-grid

A React Grid/Tree Component written in the Redux Pattern
http://react-redux-grid.herokuapp.com/
MIT License
446 stars 63 forks source link

Correct PropType constraint on Grid 'data' prop #217

Closed coatsbj closed 5 years ago

codecov-io commented 5 years ago

Codecov Report

Merging #217 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #217   +/-   ##
=======================================
  Coverage   75.71%   75.71%           
=======================================
  Files         111      111           
  Lines        5107     5107           
  Branches      366      366           
=======================================
  Hits         3867     3867           
  Misses        962      962           
  Partials      278      278
Impacted Files Coverage Δ
src/components/Grid.jsx 80% <ø> (ø) :arrow_up:

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 d3cc1f2...4870b34. Read the comment docs.

coatsbj commented 5 years ago

Note that, in order to increase test coverage of this bug, prop type failures must be treated as test failures.

Google suggests the following:

const error = console.error;
console.error = function(warning, ...args) {
    if (/(Invalid prop|Failed prop type)/.test(warning)) {
        throw new Error(warning);
    }
    error.apply(console, [warning, ...args]);
};

I am not sure whether this is something we'd want to add to karma.config.js or something (I suspect not); but I added the following locally to check for other test failures. There are a number of other tests which are failing because required props are not being supplied and/or invalid values are being supplied. I can correct these as well if desired.

bencripps commented 5 years ago

If you're up for fixing those issues, I think that's great! Otherwise I can merge this in as is, since it's an improvement.

coatsbj commented 5 years ago

Could you merge this? I started going down that rabbit hole yesterday, but the changes are somewhat cascading, and I have a team waiting on this to avoid forking. Be happy to submit a follow-up PR with those changes.

bencripps commented 5 years ago

Published as 5.6.1.