[datetime] remove moment dependency #2117

giladgray · 2018-02-09T00:21:26Z

Fixes #1930

⚠️ This PR is still a work-in-progress. Expecting test failures, but I'd like to start getting feedback on API changes.

Checklist

Fix DateInput tests
Fix DateRangeInput tests
Add tests for new methods
Update documentation
~~format.placeholder or~~ top-level placeholder prop, etc.
~~can I remove any component state fields?~~ if it ain't broke
- lots of strings stored in state, but we can probably just formatDate() at render time

Changes proposed in this pull request:

new IDateFormatProps interface extended by DateInput and DateRangeInput
- 🔥 new required formatDate and parseDate callbacks
- 🔥 format prop is now just an optional string that is passed to callbacks above
- 🔥 placeholder prop is now on the component itself (nee format.placeholder)
remove moment dependency from datetime package
- docs-app still brings it in and uses it in examples
delete all moment-based DateUtils
refactor DateInput and DateRangeInput to use IDateFormatter instead of moment

Reviewers should focus on:

functionality parity with http://blueprintjs.com/docs/v2/#datetime/dateinput and http://blueprintjs.com/docs/v2/#datetime/daterangeinput

RadioGroup allows number selectedValue (because Radio value does)

blueprint-bot · 2018-02-09T00:22:27Z

refactor DateRangeInput to use formatter instead of moment

Preview: documentation | landing | table

themadcreator · 2018-02-09T00:22:58Z

What's the total bundle size reduction?

giladgray · 2018-02-09T01:07:23Z

@adidahiya thoughts on borrowing r-d-p's DayPickerInput API? http://react-day-picker.js.org/api/DayPickerInput#formatDate

format?: string, formatDate: fn, parseDate: fn as top-level props, instead of object.

update: i did this. it's nice.

adidahiya · 2018-02-09T01:14:26Z

packages/core/src/components/forms/radioGroup.tsx

@@ -49,7 +49,7 @@ export interface IRadioGroupProps extends IProps {
    options?: IOptionProps[];

    /** Value of the selected radio. The child with this value will be `:checked`. */
-    selectedValue?: string;
+    selectedValue?: string | number;


remove unrelated change

oh but it is related, or i wouldn't have pushed it. for FormatSelect, which uses the index in an array. Radio supports number values, so RadioGroup should do.

separate PR please

giladgray · 2018-02-09T01:23:22Z

packages/datetime/test/dateInputTests.tsx

+    dateToString: date => date.toLocaleString(),
+    placeholder: "enter date",
+    stringToDate: str => new Date(Date.parse(str)),
+};


big problems with this formatter. Date.parse(str) uses UTC timezone for "YYYY-MM-DD" strings, so some dates come out a day earlier than expected. should I just use a moment formatter in these tests?

new Date("2/1/2015") > "Sun Feb 01 2015 00:00:00 GMT-0800 (PST)" new Date("2015-02-01") > "Sat Jan 31 2015 16:00:00 GMT-0800 (PST)"

…he component level; used in DateInput + DateRangeInput

giladgray · 2018-02-09T02:47:03Z

@themadcreator there's no immediate change in bundle size as datetime isn't bundled with its dependencies and moment is still used in docs-app.

this change makes moment not required when using datetime, and according to their website moment is 16.3kb gzipped.

blueprint-bot · 2018-02-09T02:54:53Z

quick-fix DRI (still needs attention)

Preview: documentation | landing | table

adidahiya

general direction lgtm. I think we should also apply this kind of change to the timezonepicker and require new timezone-related props for that component. then we can pull it back into @blueprintjs/datetime and remove the new package we made.

adidahiya · 2018-02-09T19:07:24Z

packages/datetime/src/dateInput.tsx

-            valueString: null,
-        };
-    }
+    public state: IDateInputState = {


are you sure it's ok to move this to the static member? why did we have it in the constructor before?

i am quite sure. typescript always inline state init in the constructor that it generates (before all the other => bound methods), so you can actually safely use this.props in public state initialization.

adidahiya · 2018-02-09T19:10:27Z

packages/datetime/src/dateInput.tsx

+        if (this.props.timePrecision == null) {
+            return false;
+        }
+        // TODO: getTime()?


resolve this TODO in this PR

adidahiya · 2018-02-09T19:12:20Z

packages/datetime/src/datePickerCore.tsx

+     * Return `false` if the string is an invalid date.
+     * Return `null` to represent the absence of a date.
+     */
+    parseDate(str: string, format?: string, locale?: string): Date | false | null;


not a huge blocker, but curious about this API decision: why would a consumer expect format in this function? if they're providing format as a prop, they already know the format they're interested in. in the usage of props.parseDate() it seems like you're passing props.format directly anyway.

prior art: http://react-day-picker.js.org/api/DayPickerInput#formatDate.

this is the only place the format prop gets used, so we could drop it from the API and push it to the user. but the nice thing about this usage is that only one set of moment utils for format/parseDate could be used for all formats.

adidahiya · 2018-02-09T19:13:34Z

packages/datetime/test/isotest.js

@@ -8,9 +8,18 @@ const React = require("react");
 const DateTime = require("../lib/cjs");

 describe("DateTime isomorphic rendering", () => {
+    const format = {


nit: formatProps

adidahiya · 2018-02-09T19:15:26Z

I didn't look at the logic very closely but I assume it's covered by the extensive test suite

blueprint-bot · 2018-02-09T21:24:13Z

fix most DRI tests

Preview: documentation | landing | table

blueprint-bot · 2018-02-09T21:47:43Z

fix all the DRI tests!

Preview: documentation | landing | table

giladgray · 2018-02-09T22:36:22Z

packages/datetime/src/dateFormat.tsx

+    /**
+     * Date format string, passed to `formatDate` and `parseDate`.
+     */
+    format?: string;


@adidahiya what's your take on this prop? worth keeping?

prefer to keep it simple and add it later if it's desired

adidahiya · 2018-02-09T23:10:25Z

code looks fine but this radio doesn't work for me:

giladgray · 2018-02-09T23:14:45Z

@adidahiya not sure what you mean with format screenshot, works for me. i just removed the (def broken) "from now" option.

blueprint-bot · 2018-02-09T23:19:01Z

remove format prop, add formatting tests

Preview: documentation | landing | table

adidahiya

nice docs

adidahiya · 2018-02-09T23:30:43Z

we should probably still add a @blueprintjs/datetime-moment package after this PR

blueprint-bot · 2018-02-09T23:30:51Z

add docs section about "Date formatting"

Preview: documentation | landing | table

blueprint-bot · 2018-02-09T23:43:48Z

add imports to docs

Preview: documentation | landing | table

cmslewis

Exciting stuff!

Gilad Gray added 7 commits February 8, 2018 15:33

remove moment dependency from datetime package

287c0cc

remove string format support, require format prop

829e221

delete all moment-based DateUtils

2ebc0f1

refactor DateInput to use IDateFormatter instead of moment

9f336a3

DateInputTests require format prop

571c7ec

refactor examples and FormatSelect

00ad293

RadioGroup allows number selectedValue (because Radio value does)

refactor DateRangeInput to use formatter instead of moment

0ddbc5d

giladgray requested review from cmslewis and adidahiya February 9, 2018 00:21

giladgray added the Status: work in progress label Feb 9, 2018

fix isotest

d48d1a1

adidahiya reviewed Feb 9, 2018

View reviewed changes

giladgray commented Feb 9, 2018

View reviewed changes

Gilad Gray added 3 commits February 8, 2018 18:42

IDateFormatProps puts format, placeholder, formatDate, parseDate at t…

aace401

…he component level; used in DateInput + DateRangeInput

fix DateInputTests, refactor DRITests to use new shared DATE_FORMAT

9d83440

update DateInput to fix tests!

7c96e4d

giladgray added the Type: breaking change label Feb 9, 2018

quick-fix DRI (still needs attention)

69b5b30

giladgray mentioned this pull request Feb 9, 2018

RadioGroup accepts number values #2119

Merged

Merge branch 'develop' of github.com:palantir/blueprint into gg/unmoment

3849da6

adidahiya reviewed Feb 9, 2018

View reviewed changes

Gilad Gray added 3 commits February 9, 2018 13:19

refactors: dateFormat file, util functions

3aabff7

add value tag to examples so you can watch it change

0bf240b

fix most DRI tests

e56625f

giladgray force-pushed the gg/unmoment branch from dc4f746 to e56625f Compare February 9, 2018 21:19

fix isotest, todo

27f8cb3

fix all the DRI tests!

a217caa

giladgray commented Feb 9, 2018

View reviewed changes

remove format prop, add formatting tests

6515048

add docs section about "Date formatting"

9c546eb

adidahiya approved these changes Feb 9, 2018

View reviewed changes

add imports to docs

f183644

cmslewis approved these changes Feb 10, 2018

View reviewed changes

giladgray merged commit 3c3a81c into develop Feb 10, 2018

giladgray deleted the gg/unmoment branch February 10, 2018 00:42

giladgray mentioned this pull request Feb 10, 2018

[datetime] Replace moment with date-fns #1952

Closed

3 tasks

adidahiya mentioned this pull request Feb 15, 2018

Consider using date-fns instead of moment in datetime package #317

Closed

giladgray mentioned this pull request Mar 3, 2018

dateInput.js:251 Uncaught TypeError: parseDate is not a function #2196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[datetime] remove moment dependency #2117

[datetime] remove moment dependency #2117

giladgray commented Feb 9, 2018 •

edited

Loading

blueprint-bot commented Feb 9, 2018

themadcreator commented Feb 9, 2018

giladgray commented Feb 9, 2018 •

edited

Loading

adidahiya Feb 9, 2018

giladgray Feb 9, 2018

adidahiya Feb 9, 2018

giladgray Feb 9, 2018

giladgray Feb 9, 2018

giladgray commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

adidahiya left a comment

adidahiya Feb 9, 2018

giladgray Feb 9, 2018

adidahiya Feb 9, 2018 •

edited by giladgray

Loading

adidahiya Feb 9, 2018

giladgray Feb 9, 2018

adidahiya Feb 9, 2018

adidahiya commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

giladgray Feb 9, 2018

adidahiya Feb 9, 2018

adidahiya commented Feb 9, 2018

giladgray commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

adidahiya left a comment

adidahiya commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

cmslewis left a comment

[datetime] remove moment dependency #2117

[datetime] remove moment dependency #2117

Conversation

giladgray commented Feb 9, 2018 • edited Loading

Fixes #1930

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

blueprint-bot commented Feb 9, 2018

refactor DateRangeInput to use formatter instead of moment

themadcreator commented Feb 9, 2018

giladgray commented Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

quick-fix DRI (still needs attention)

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Feb 9, 2018 • edited by giladgray Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

fix most DRI tests

blueprint-bot commented Feb 9, 2018

fix all the DRI tests!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Feb 9, 2018

giladgray commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

remove format prop, add formatting tests

adidahiya left a comment

Choose a reason for hiding this comment

adidahiya commented Feb 9, 2018

blueprint-bot commented Feb 9, 2018

add docs section about "Date formatting"

blueprint-bot commented Feb 9, 2018

add imports to docs

cmslewis left a comment

Choose a reason for hiding this comment

giladgray commented Feb 9, 2018 •

edited

Loading

giladgray commented Feb 9, 2018 •

edited

Loading

adidahiya Feb 9, 2018 •

edited by giladgray

Loading