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

Provide a simple moment-less API #382

Closed
rafeememon opened this issue Mar 3, 2016 · 8 comments
Closed

Provide a simple moment-less API #382

rafeememon opened this issue Mar 3, 2016 · 8 comments

Comments

@rafeememon
Copy link
Contributor

In one of my projects I'm passing dates to server API calls as strings, and I also receive dates back as strings which are stored in redux state. In the spirit of keeping my state clean I keep the strings as the source of truth rather than moment objects; this also makes it possible to pre-generate my state on the server, which would get ugly if it contained moment objects. This led me to write a wrapper datepicker component that would simply translate between strings and moment objects so that they can be passed as props to the datepicker. This got me thinking that maybe we should provide a way of dealing with strings natively in this component. I can think of a couple approaches here:

  1. Provide wrapper component in datepicker -- Only functionality would be to wrap/unwrap moment objects and pass them to the real datepicker component. Probably the easiest to maintain, cleanest code-wise; but calling something SimpleDatePicker seems a little silly
  2. Provide a simple prop -- When set all props (selected, minDate, etc, the value passed to onChange) will be strings formatted according to dateFormat. A little dirtier code-wise; adding a prop to control how other props are handled seems kind of anti-react to me
  3. Automatically determine behavior -- Determine the intended behavior by what is passed into selected: if a string, everything is a string, and if a moment, everything is a moment. By far the simplest API; dirtier code-wise, and I'm not sure how it should behave when selected is null

What does everyone think?

@rafeememon
Copy link
Contributor Author

For posterity here is what my wrapper component looks like:

import React from 'react'
import DatePicker from 'react-datepicker'
import moment from 'moment'

const DATE_FORMAT = 'MM/DD/YYYY'

export default class SimpleDatePicker extends React.Component {
  static propTypes = {
    selected: React.PropTypes.string,
    startDate: React.PropTypes.string,
    endDate: React.PropTypes.string,
    minDate: React.PropTypes.string,
    maxDate: React.PropTypes.string,
    onChange: React.PropTypes.func.isRequired
  };

  getMoment (string) {
    return string && moment(string, DATE_FORMAT)
  }

  onChange (date) {
    this.props.onChange(date && date.format(DATE_FORMAT))
  }

  render () {
    return <DatePicker
      {...this.props}
      dateFormat={DATE_FORMAT}
      selected={this.getMoment(this.props.selected)}
      startDate={this.getMoment(this.props.startDate)}
      endDate={this.getMoment(this.props.endDate)}
      minDate={this.getMoment(this.props.minDate)}
      maxDate={this.getMoment(this.props.maxDate)}
      onChange={::this.onChange} />
  }
}

@martijnrusschen
Copy link
Member

  1. Feels silly but keeps it simple
  2. Sounds nice, but feels wrong.
  3. This is probably the best UX, but I don't think this is really stable and reliable in the future.

This comment isn't really helping, but wanted to share my thoughts.

@greaber
Copy link

greaber commented Jul 25, 2016

IMO, since a date picker just lets you choose a year, month, and day, its state should just be three JavaScript numbers. The component is just a piece of UI and shouldn't be prescriptive about your data model. It shouldn't require a heavy dependency like moment.js or even use JavaScript dates. You can keep whatever you want in Redux -- triples of numbers, JavaScript dates, moment dates, or even strings. Also, instead of adding timezone awareness as suggested in #507 and #498, it could just allow the user to explicitly pass a value for "today" (which would also be a triple of numbers).

@aij
Copy link
Contributor

aij commented Mar 17, 2017

The more I look at this, the more I think a better solution would be the opposite of 1: Make the core API as simple as possible (but no simpler), and add a wrapper around that which adds moment, timezones, timestamps and any other unnecessary but potentially convenient complexity.

@ro-savage
Copy link
Contributor

ro-savage commented Sep 24, 2017

I used react-datepicker in a previous project and did some PRs. It's a really nice datetime picker.

However, the moment dependency is stopping me from using it again. We have no need for moment and its absolutely huge.

Would love to see moment become optional. Any updates thoughts @martijnrusschen ?

@martijnrusschen
Copy link
Member

All open for getting rid of moment. I don't have time to work on that currently, but I'm welcoming PR's to make this simpler.

@prayerslayer
Copy link
Contributor

prayerslayer commented Sep 30, 2017

Hi, I'd like to give it a shot as part of Hacktoberfest. Making big dependencies optional is always nice, because while I'm sure I'll need such a component at some point I'd definitely search for something else due to moment :)

Are there requirements beyond 1.) getting rid of moment and 2.) maintaining backwards-compatibility (ie. not break anything)? Any part of the code that I should pay special attention to because it's complex, has non-obvious side effects, is not as well tested as other parts, or something like that?

I looked a bit over the source and would approach it like this:

  1. PR that moves all usages of moment into a separate module, possibly just date_utils
  2. PR that introduces another version of this module that uses date-fns

Then we can decide how to build the API to use one or the other. Simplest possibility is a separate build?

@martijnrusschen
Copy link
Member

Moment was removed in #1527

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

6 participants