Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflicts with moment-parseplus #35

Closed
nwolverson opened this issue Apr 26, 2018 · 6 comments
Closed

Conflicts with moment-parseplus #35

nwolverson opened this issue Apr 26, 2018 · 6 comments
Assignees

Comments

@nwolverson
Copy link

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 ddc0b2e affecting moment runtime behaviour is required.

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

@CalebMorris
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

CalebMorris commented Apr 29, 2018

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 added a commit that referenced this issue Apr 29, 2018
This allows consumers to choose how they wish to handle this behavior.
CalebMorris added a commit that referenced this issue Apr 29, 2018
This allows consumers to choose how they wish to handle this behavior.
CalebMorris added a commit that referenced this issue Apr 29, 2018
This allows consumers to choose how they wish to handle this behavior.
@CalebMorris
Copy link
Owner

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
Copy link
Author

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

CalebMorris added a commit that referenced this issue Jun 9, 2018
This allows consumers to choose how they wish to handle this behavior.
CalebMorris added a commit that referenced this issue Jun 9, 2018
This allows consumers to choose how they wish to handle this behavior.
@CalebMorris
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants