CalebMorris / react-moment-proptypes

React proptype validator for moment.js
49 stars 17 forks source link

object `isRequired` not functioning as expected with explicit `null` or `undefined` #23

Closed majapw closed 7 years ago

majapw commented 7 years ago

Hi @CalebMorris! I tested this by cloning your repository and making some modifications to your test suite.

Basically, in your isRequired test for momentObj, you have the following:

 describe('Missing required object', () => {
    before(() => {
      TestClass = React.createClass({
        propTypes : {
          testRequiredObject : MomentPropTypes.momentObj.isRequired,
        },
        render() {
          return null;
        },
      });
    });

    it('should have a warning for the missing moment obj', (done) => {
      const testElement = <TestClass />;
      TestUtils.renderIntoDocument(testElement);

      expect(warnings).to.be.an('array');
      expect(warnings.length).to.equal(1);
      expect(warnings[0]).to.contain('Required prop');
      done();
    });
});

Which of course, does what you expect it to and raises a warning. However, when you change the test element to be <TestClass testRequiredObject={null} /> or <TestClass testRequiredObject={undefined} />, you find that the test no longer raises an warning even though I would expect it to.

FYI: @ljharb

funwithtriangles commented 7 years ago

I've noticed this behaviour in my own app.

CalebMorris commented 7 years ago

Confirmed. I'll fix the behavior as soon as I have some free time to work on it (likely by Saturday).

majapw commented 7 years ago

Thanks @CalebMorris! Let me know how I can help. :)

CalebMorris commented 7 years ago

Added this behavior in v1.4.0. 2 things to note with this change.

  1. The content of the isRequired warning now matches the common React PropTypes for this behavior.
  2. Found an odd behavior bug in my integration tests so I created #26 to investigate further so this change is covered for 2/3 cases (<Foo bar={undefined} failing in console, but not browser).