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

Optional moment.js Part 1 #1046

Merged
merged 11 commits into from
Oct 3, 2017

Conversation

prayerslayer
Copy link
Contributor

@prayerslayer prayerslayer commented Sep 30, 2017

Part 1 of #382

  • Moves all calls on moment or instances of moment into date_utils. (That I found, at least. Since most date properties are still instances of moment I might have missed some.)
  • Discussable if there are too many functions and how the moment-agnostic Date API should look like.
  • No efforts regarding order of functions were made, this I'll do later but I still wanted to give you the change to look at it.

After this PR I would work on replacing the actual moment instances.

@codecov
Copy link

codecov bot commented Sep 30, 2017

Codecov Report

Merging #1046 into master will increase coverage by 1.83%.
The diff coverage is 96.89%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1046      +/-   ##
=========================================
+ Coverage   81.27%   83.1%   +1.83%     
=========================================
  Files          13      13              
  Lines         721     817      +96     
  Branches      142     141       -1     
=========================================
+ Hits          586     679      +93     
  Misses         33      33              
- Partials      102     105       +3
Impacted Files Coverage Δ
src/year_dropdown.jsx 87.8% <100%> (+0.62%) ⬆️
src/month.jsx 81.25% <100%> (-0.57%) ⬇️
src/week.jsx 72.72% <100%> (-0.61%) ⬇️
src/calendar.jsx 83.6% <100%> (-0.53%) ⬇️
src/month_dropdown.jsx 85% <100%> (-2.18%) ⬇️
src/day.jsx 87.64% <66.66%> (+0.43%) ⬆️
src/time.jsx 52.27% <88.88%> (-2.28%) ⬇️
src/datepicker.jsx 90.39% <94.73%> (+0.77%) ⬆️
src/date_utils.js 91.44% <99%> (+11.78%) ⬆️
... and 7 more

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 99d6879...f9a44b0. Read the comment docs.

Copy link
Member

@martijnrusschen martijnrusschen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, would be good if someone else could take a look as well.

Copy link
Contributor

@aij aij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too.

I don't think we could completely remove moment while maintaining backwards compatibility, but we could probably make it an optional dependency.

@prayerslayer
Copy link
Contributor Author

prayerslayer commented Oct 1, 2017

I don't think we could completely remove moment while maintaining backwards compatibility

Yeah me neither, unless you'd distribute separate builds for versions with and without moment. If that's not an option we'd need a flag somewhere to know which date utils to use and, as you say, moment as an optional dependency.

@prayerslayer
Copy link
Contributor Author

Any idea what could be wrong with the failing test? It seems to be occuring also on master (see this build https://travis-ci.org/prayerslayer/react-datepicker/builds/281940187), so I assume it's a dependency issue?

I'll continue on a branch based on this PR instead of master.

@aij
Copy link
Contributor

aij commented Oct 2, 2017

@prayerslayer It appears to be a fragile test. It looks like it will always fail on the first 2 days of the month.

@aij aij merged commit f3c9d89 into Hacker0x01:master Oct 3, 2017
@aij aij mentioned this pull request Mar 2, 2018
@montogeek
Copy link

Any updates on this?

@prayerslayer
Copy link
Contributor Author

@montogeek There's this #1327, but it's quite old by now, and has some blocking open questions.

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

Successfully merging this pull request may close these issues.

4 participants