CalebMorris / react-moment-proptypes

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

Conflicts with moment-parseplus #35

Closed nwolverson closed 6 years ago

nwolverson commented 6 years ago

When I added react-dates to my project, the dependency on react-moment-proptypes broke the use of moment-parseplus, because both override moment.createFromInputFallback. The flaky global override certainly seems to be an issue arising from both projects/moment design, but moment-parseplus documents this up front, while react-moment-proptypes does not, not clear to me why this change https://github.com/CalebMorris/react-moment-proptypes/commit/ddc0b2ed0cdb0cee7ce8b69eee3c009545a9e22d affecting moment runtime behaviour is required.

(Workaround: change ordering to overwrite in the other direction)

CalebMorris commented 6 years ago

The added the moment.createFromInputFallback as a workaround to suppress the annoying message that shows up if you ever use the default moment constructor:

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.
Arguments: 
[0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: not a date, _f: undefined, _strict: undefined, _locale: [object Object]
Error
    at [... stack trace]

Can you provide a code snippet show what behavior is actually occurring? I'm not sure what you mean by broke the use of moment-parseplus? When testing using both I don't see any problems.

Attempted case:

var moment = require('moment');
require('moment-parseplus');

var date = moment('March 5th, 2016');
console.log(date);
console.log(moment());
nwolverson commented 6 years ago
require('moment-parseplus');
require('react-moment-proptypes') // or e.g. react-dates which depends on this

var date = moment('March 5th, 2016');
// moment.invalid(/* March 5th, 2016 */)

The "annoying message" is an intentional "feature" of moment, I can completely understand suppressing it but I'm not why a proptypes validation library would be doing so - an end-user app, sure.

CalebMorris commented 6 years ago

I understand that it's not a good thing to leave into a production library. I'll work towards changing it as part of a solution here.

I'm not seeing any problem with the code snippet you've given (no console message or failure when using a moment instance). Can you please specify the version in your package-lock.json and what behavior you are seeing as an indication of the conflict?

CalebMorris commented 6 years ago

I create #40 to change this behavior and update this library's documentation. I want to confirm it fixes the problem interacting with moment-parseplus before merging and releasing, though

nwolverson commented 6 years ago

Apologies for the delay - that looks good to me, just tried that out and works fine for me after removing workaround

CalebMorris commented 6 years ago

Fix (among other changes) released in v1.6.0.