Convert timestamp component to React #1088

robertknight · 2019-04-23T13:52:04Z

(There is no urgency on this PR, I had it sitting around on my system waiting for #1061 to be merged)

A notable change is that the "current" time used to generate the relative
timestamp is now stored as an explicit piece of state, rather than
implicitly using the current time when calling toFuzzyString. This
provides a clean way to trigger a re-render of timestamp periodically:
an effect updates the "now" time, and that triggers a recalculation of
the relative timestamp string.

The tests have also been revised to better capture the user-facing
behavior of the component.

A notable change is that the current time used to generate the relative timestamp is now stored as an explicit piece of state, rather than implicitly using the current time when calling `toFuzzyString`. This provides a clean way to trigger a re-render of timestamp periodically: an effect updates the "now" time, and that triggers a recalculation of the timestamp. The tests have also been revised to better capture the user-facing behavior of the component.

codecov · 2019-04-23T13:55:00Z

Codecov Report

Merging #1088 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
+ Coverage   92.51%   92.52%   +0.01%     
==========================================
  Files         164      164              
  Lines        6343     6340       -3     
  Branches     1028     1028              
==========================================
- Hits         5868     5866       -2     
+ Misses        475      474       -1

Impacted Files	Coverage Δ
src/sidebar/util/time.js	`97.59% <100%> (+0.02%)`	⬆️
src/sidebar/components/timestamp.js	`100% <100%> (+4.54%)`	⬆️

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 2b76345...5957b12. Read the comment docs.

hmstepanek

Hi Robert, functionally this looks good. I had some thoughts on refactoring/improving the readability of the tests.

hmstepanek · 2019-05-07T17:14:27Z

src/sidebar/util/time.js

 * @return {string} A 'fuzzy' string describing the relative age of the date.
 */
-function toFuzzyString(date, Intl) {
+function toFuzzyString(date, now = new Date(), Intl) {


It's weird to me that this works but I guess that's a difference between js and python. In python this wouldn't create a new Date every time but looks like that's not the case in js.

What is Intl here-shouldn't that be documented as a param?

Intl here is the browser's Intl global object which is a namespace for internationalization objects. Yes it should be documented :P

It's weird to me that this works but I guess that's a difference between js and python.

Yes indeed. In JS the default value is evaluated on every call if the argument is not assigned. I think this is much more useful than Python 😉

hmstepanek · 2019-05-07T17:25:24Z

src/sidebar/util/test/time-test.js

@@ -109,7 +109,7 @@ describe('sidebar.util.time', function() {
      const d = new Date().toISOString();
      sandbox.clock.tick(year * 2 * 1000);

-      assert.equal(time.toFuzzyString(d, null), 'Thu Jan 01 1970');
+      assert.equal(time.toFuzzyString(d, undefined, null), 'Thu Jan 01 1970');


Seems like we should be taking advantage of being able to pass the date to compare against as an argument in these tests now. It would make these tests a lot more explicit and easier to understand.

hmstepanek · 2019-05-07T17:35:42Z

src/sidebar/util/test/time-test.js

    });

    const testFixture = function(f) {
      return function() {
        const t = new Date().toISOString();
        const expect = f[1];
        sandbox.clock.tick(f[0] * 1000);
-        assert.equal(time.toFuzzyString(t, mockIntl()), expect);
+        assert.equal(time.toFuzzyString(t, undefined, mockIntl()), expect);


I find this really hard to follow and have a couple thoughts about how to fix that. Moving the test cases to the location where they are used would be helpful here in terms of readability, also passing the params explicitly by name here instead of using f[0], f[1]. If we pass in the time to the toFuzzyString rather than undefined we won't have to tick the clock and this test will be much easier to read IMO.

robertknight · 2019-05-08T11:33:11Z

@hmstepanek volunteered to address the issues she raised during code review, so I've assigned to her.

robertknight · 2019-05-29T12:39:38Z

Hi @hmstepanek - Can you help me get this merged? The tests that you commented that you want to refactor are only lightly touched by this PR for the purposes of being able to test the timestamp component. If you have time to look at them then great, otherwise can that be addressed separately?

hmstepanek · 2019-05-29T15:28:48Z

Ah yes let me do this now!

robertknight · 2019-05-31T11:34:33Z

src/sidebar/util/test/time-test.js

    }
+    fakeIntl = sinon.stub().returns({


Intl is a namespace rather than a function. In the browser you access it via Intl.DateTimeFormat rather than say Intl().DateTimeFormat or new Intl().DateTimeFormat.

robertknight

I can't leave "Request changes" or "Approval" feedback on this PR because it is an extension of my work in the first place. I've left a few notes on the last commit though.

robertknight · 2019-05-31T11:40:46Z

src/sidebar/util/test/time-test.js

+      it('creates correct fuzzy string for fixture ' + test.time, () => {
+        const epoch = fakeDate('1970-01-01T00:00:00.000Z');
+        const epochPlusTimeStamp = new Date(test.time);
+        assert.equal(time.toFuzzyString(epoch, epochPlusTimeStamp), test.text);


I found the variable names confusing here. What is important is not that the first argument happens to be the epoch and the second value happens to be shortly afterwards, but that the first argument is the timestamp you want to express as a relative date and the second argument is the "current' time.

robertknight · 2019-05-31T11:42:32Z

src/sidebar/util/test/time-test.js

-    }
+    [
+      {
+        time: '1970-01-02T03:00:00.000Z',


I think time is ambiguous here. Does it mean the current time or the timestamp you want to format? currentTime or now might be better.

robertknight · 2019-05-31T11:44:30Z

src/sidebar/util/time.js

@@ -140,9 +146,6 @@ const BREAKPOINTS = [
 ];

 function getBreakpoint(date, now) {


Could you add JSDoc comments to getBreakpoint to clarify the expected types.

robertknight · 2019-05-31T11:45:00Z

src/sidebar/util/time.js

@@ -140,9 +146,6 @@ const BREAKPOINTS = [
 ];

 function getBreakpoint(date, now) {
-  // Turn the given ISO 8601 string into a Date object.
-  date = new Date(date);
-
  let breakpoint;


Please update the JSDoc comments here to reflect the expected types after your changes.

robertknight · 2019-05-31T11:55:29Z

src/sidebar/util/test/time-test.js

+    ].forEach(test => {
+      it('creates correct fuzzy string for fixture ' + test.time, () => {
+        const epoch = fakeDate('1970-01-01T00:00:00.000Z');
+        const epochPlusTimeStamp = new Date(test.time);


You're using a date with a mock getFullYear function to represent the timestamp being formatted but a normal Date to represent the current date - where the output can depend on the current timezone.

robertknight · 2019-05-31T12:28:41Z

src/sidebar/components/timestamp.js

+
+  // Fuzzy, relative timestamp (eg. '6 days ago')
+  const prevTime = timestamp ? new Date(timestamp) : null;
+  const relativeTimestamp = useMemo(() => toFuzzyString(prevTime, now), [


If you move the prevTime calculation inside the useMemo callback, that will save a little bit of work because the timestamp will only be parsed into a Date when it changes, rather than every time the component is rendered. Probably completely immaterial compared to all the other work that is happening, but I thought I'd flag it up as an unintended change.

hmstepanek · 2019-06-03T16:37:24Z

Ok @robertknight. This is ready for a detailed review now.

robertknight

I left some small suggestions. I can't approve this since it was my PR originally. Can I leave it to you to do that and merge if you're happy with it @hmstepanek ?

robertknight · 2019-06-03T16:42:47Z

src/sidebar/util/test/time-test.js

+      },
+    ].forEach(test => {
+      it(
+        'passes correct arguments to format fuzzy string date for fixture ' +


Suggested change

'passes correct arguments to format fuzzy string date for fixture ' +

'passes correct arguments to `Intl.DateTimeFormat.format` for fixture ' +

robertknight · 2019-06-03T16:43:14Z

src/sidebar/util/test/time-test.js

+
+          fakeIntl.DateTimeFormat().format.returns(test.text); // eslint-disable-line new-cap
+          assert.equal(time.toFuzzyString(timeStamp, now, fakeIntl), test.text);
+          assert.calledWith(fakeIntl.DateTimeFormat, undefined, test.options);


These two lines can be combined into one calledWith check

How are you thinking of combining these together? I'm afraid I'm not seeing it. I could see just getting rid of the first check all-together except that we might perhaps still want to check to see that the value from the format function was actually returned.

Please ignore this. I misread the two "calledWith" asserts as having the same first argument. I've just realized that is not the case.

robertknight · 2019-06-03T16:44:01Z

src/sidebar/util/test/time-test.js


-      assert.equal(time.toFuzzyString(d, null), 'Thu Jan 01 1970');
+      assert.equal(time.toFuzzyString(timeStamp, now, null), 'Thu Jan 01 1970');
+      assert.calledOnce(timeStamp.toDateString);


You probably want to reverse the order of these asserts since the first one will lead to the second one.

robertknight · 2019-06-03T16:44:28Z

src/sidebar/util/test/time-test.js


-      assert.equal(time.toFuzzyString(d, null), 'Thu Jan 01 1970');
+      assert.equal(time.toFuzzyString(timeStamp, now, null), 'Thu Jan 01 1970');
+      assert.calledOnce(timeStamp.toDateString);


Same here, I think it makes sense to check what happens in the function before the return value, and then the return value

robertknight · 2019-06-03T16:50:14Z

src/sidebar/util/time.js

- * @param {number} date - The absolute timestamp to format.
+ * @param {Date} date - The date to consider as the timestamp to format.
+ * @param {Date} now - The date to consider as the current time.
+ * @param {Object} Intl - The internationalization.


Suggested change

* @param {Object} Intl - The internationalization.

* @param {Object} Intl - JS internationalization API implementation.

- Use now variable instead of walking the clock. - Use iso format instead of seconds. - Pass a Date object instead of an iso format to time.<methods>so the methods off the Date object can be mocked in order to avoid timezone specific tests. Since methods like getFullYear output the year in whatever timezone the operating system is set to, these methods must be mocked/mapped to their UTC equivalents when testing such as getUTCFullYear. - Remove offseting of time based on timezone as this is now covered by mocking the getFullYear method and not needed for the decayInterval as that only cares about time deltas.

hmstepanek self-requested a review May 7, 2019 02:28

hmstepanek suggested changes May 7, 2019

View reviewed changes

robertknight assigned hmstepanek May 8, 2019

robertknight commented May 31, 2019

View reviewed changes

hmstepanek force-pushed the react-timestamp branch 4 times, most recently from a745df8 to dedd3cc Compare June 3, 2019 16:34

robertknight commented Jun 3, 2019

View reviewed changes

hmstepanek force-pushed the react-timestamp branch from dedd3cc to 5957b12 Compare June 4, 2019 16:43

hmstepanek approved these changes Jun 4, 2019

View reviewed changes

hmstepanek merged commit 237bef7 into master Jun 4, 2019

hmstepanek deleted the react-timestamp branch June 4, 2019 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert timestamp component to React #1088

Convert timestamp component to React #1088

robertknight commented Apr 23, 2019

codecov bot commented Apr 23, 2019 •

edited

Loading

hmstepanek left a comment

hmstepanek May 7, 2019

hmstepanek May 7, 2019

robertknight May 7, 2019

robertknight May 7, 2019 •

edited

Loading

hmstepanek May 7, 2019

hmstepanek May 7, 2019

robertknight commented May 8, 2019

robertknight commented May 29, 2019

hmstepanek commented May 29, 2019

robertknight May 31, 2019

robertknight left a comment

robertknight May 31, 2019

robertknight May 31, 2019

robertknight May 31, 2019

robertknight May 31, 2019

robertknight May 31, 2019

robertknight May 31, 2019

hmstepanek commented Jun 3, 2019

robertknight left a comment

robertknight Jun 3, 2019

robertknight Jun 3, 2019

hmstepanek Jun 3, 2019

robertknight Jun 4, 2019

robertknight Jun 3, 2019

robertknight Jun 3, 2019

robertknight Jun 3, 2019

		@@ -140,9 +146,6 @@ const BREAKPOINTS = [
		];

		function getBreakpoint(date, now) {

	'passes correct arguments to format fuzzy string date for fixture ' +
	'passes correct arguments to `Intl.DateTimeFormat.format` for fixture ' +

	* @param {Object} Intl - The internationalization.
	* @param {Object} Intl - JS internationalization API implementation.

Convert timestamp component to React #1088

Convert timestamp component to React #1088

Conversation

robertknight commented Apr 23, 2019

codecov bot commented Apr 23, 2019 • edited Loading

Codecov Report

hmstepanek left a 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

robertknight May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented May 8, 2019

robertknight commented May 29, 2019

hmstepanek commented May 29, 2019

Choose a reason for hiding this comment

robertknight left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmstepanek commented Jun 3, 2019

robertknight left a 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

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

codecov bot commented Apr 23, 2019 •

edited

Loading

robertknight May 7, 2019 •

edited

Loading