Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

fiennyangeln · 2018-10-08T16:30:52Z

Motivation

Fix #1015

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

run `git log --follow --summary [some file relative path]
see the result and capture the last date where commit is about content change not rename / move
Run the localhost for v1 (cd v1 && yarn start), check in the site

Related PRs

No

docusaurus-bot · 2018-10-08T16:37:02Z

Deploy preview for docusaurus-preview ready!

Built with commit b38dd21

https://deploy-preview-1023--docusaurus-preview.netlify.com

or otherwise when it is first created Fix facebook#1015

yangshun

This looks awesome @fiennyangeln! I have some minor comments but we're good to land this after they're addressed.

yangshun · 2018-10-08T20:41:49Z

v1/lib/core/utils.js

-  const timeSpan = spawn
-    .sync('git', ['log', '-1', '--format=%ct', filepath])
-    .stdout.toString('utf-8');
+  // To differentiate between content change and file renaming / moving, use --summary


Sorry for only noticing this now, but we should add a try/catch around the code within getGitLastUpdated and return null in the catch case something blows up (for instance, git is not installed on the device, or if the repository is a Mercurial-based one).

Actually im hoping we remove cross-spawn dependency and use existing shelljs to call the bash command.

@endiliey yeah i see some v2 code uses shell.exec()

V1 too. Check v1/lib/publish-pages.js

yangshun · 2018-10-09T00:11:17Z

v1/lib/core/utils.js

@@ -38,10 +38,37 @@ function idx(target, keyPaths) {
  );
 }

+function isNormalInteger(str) {
+  return /^\+?(0|[1-9]\d*)$/.test(str);


Use this regex instead: ^\d+$, which is simpler. This regex means the line only contains integers. Check out https://regex101.com/r/mp3HYD/2/

It should be noted that above (fienny's PR) allow an optional + at the beginning of string

Yangshun's recommended regex doesnt allow that.

If the optional + is intended, use /^\+?\d+$/

Wondering if there is normal and abnormal integer from the function naming.

isInt is fine o.o

The numbers are timestamps, why do we need the +? 🤔

@yangshun yup i dont think we need the +

yangshun · 2018-10-09T00:17:07Z

v1/lib/core/utils.js

+    .split('\n')
+    .filter(String);
+
+  const timeSpan = records.find(


This looks right, but could you test on a file that only has one commit?

Could you also test on files that have 0 commits, aka newly-created files that haven't been checked into Git yet. I suspect we this part might crash.

We don't want the page to blow up for any newly-created pages else they wouldn't be able to preview it.

We currently have test that check for file with only one commit, non existing and those with no git timestamp in here v1/lib/core/__tests__/utils.test.js

yangshun · 2018-10-09T00:21:08Z

v1/lib/core/utils.js

+    .filter(String);
+
+  const timeSpan = records.find(
+    (item, index, arr) =>


I'd rewrite this part as such to make the logic clearer:

const timeSpan = records.find((item, index, arr) => { const isTimestamp = isNormalInteger(item); const isLastItem = index + 2 <= arr.length; // Note: Let's use <= instead of === to be safer. const nextItemIsTimestamp = isNormalInteger(arr[index + 1]); return isTimestamp && (isLastItem || nextItemIsTimestamp); });

I think if we use <= it will return true for all element except the last

endiliey

See review

- Uses shelljs instead of cross-spawn - Make logic clearer

JoelMarcey

After a quick chat with @endiliey and @yangshun, we probably don't want to execute git commands in directly within the unit tests.

@yangshun - I like the mocking idea. Could you provide a suggestion to @fiennyangeln on how to do that?

yangshun · 2018-10-11T18:06:19Z

v1/lib/core/__tests__/utils.test.js

+    const tempFilePath2 = path.join(__dirname, '__fixtures__', '.temp2');
+    fs.writeFileSync(tempFilePath2, 'Lorem ipsum :)');
+
+    shell.exec(`git add ${tempFilePath2}`);


Don't execute shell commands in tests. Use Jest mocking to mock out the entire shell module and the methods that your method calls.

Okay!!! Will try it :D

yangshun · 2018-10-11T18:07:00Z

@JoelMarcey Have left some comments 😄

Thanks for your patience @fiennyangeln!

I initially try to mock the whole shelljs. But it returns error shell.exec is not a function when i try to provide the mockResolvedValue I think it is because of the inner code of shelljs who run a forEach to require each of its method which make it a promise. I tried moving the jest.mock inside beforeAll and also adding babel-dynamic-import but it did not solve the problem. In the end, I decided to just mock shelljs.exec since it is the only function used anyway

fiennyangeln · 2018-10-14T09:16:03Z

I initially try to mock the whole shelljs. But it returns error shell.exec is not a function when i try to provide the mockResolvedValue. I think it is because of the inner code of shelljs who run a forEach to
require each of its method which make it a promise. I tried moving the jest.mock inside beforeAll and also adding babel-dynamic-import but it did not solve the problem. In the end, I decided to just mock shelljs.exec since it is the only function used anyway

fiennyangeln · 2018-10-14T13:07:02Z

@yangshun please review :D

yangshun

LGTM. @fiennyangeln Thanks for fixing! There are some small improvements that can be made but I'll address them 😄

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 8, 2018

Fix last updated time misleading, only show when file content change

26957af

or otherwise when it is first created Fix facebook#1015

fiennyangeln force-pushed the fix/last-updated-time branch from f736f9c to 26957af Compare October 8, 2018 16:45

Fix prettier

5d1e552

JoelMarcey requested a review from yangshun October 8, 2018 17:05

yangshun suggested changes Oct 9, 2018

View reviewed changes

endiliey suggested changes Oct 9, 2018

View reviewed changes

- Simplify regex

100b117

- Uses shelljs instead of cross-spawn - Make logic clearer

fiennyangeln force-pushed the fix/last-updated-time branch 3 times, most recently from c10ef21 to d0eab47 Compare October 10, 2018 16:09

Add test when repositories is moved

e3f3ca3

fiennyangeln force-pushed the fix/last-updated-time branch from d0eab47 to e3f3ca3 Compare October 10, 2018 16:14

JoelMarcey suggested changes Oct 11, 2018

View reviewed changes

yangshun reviewed Oct 11, 2018

View reviewed changes

yangshun approved these changes Oct 14, 2018

View reviewed changes

yangshun merged commit a39677c into facebook:master Oct 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

fiennyangeln commented Oct 8, 2018

docusaurus-bot commented Oct 8, 2018 •

edited

Loading

yangshun left a comment

yangshun Oct 8, 2018

endiliey Oct 9, 2018

fiennyangeln Oct 10, 2018

endiliey Oct 10, 2018

yangshun Oct 9, 2018

endiliey Oct 9, 2018

endiliey Oct 9, 2018 •

edited

Loading

yangshun Oct 10, 2018

fiennyangeln Oct 10, 2018

yangshun Oct 9, 2018

fiennyangeln Oct 10, 2018 •

edited

Loading

yangshun Oct 9, 2018

fiennyangeln Oct 10, 2018

endiliey left a comment •

edited

Loading

JoelMarcey left a comment

yangshun Oct 11, 2018

fiennyangeln Oct 13, 2018

yangshun commented Oct 11, 2018

fiennyangeln commented Oct 14, 2018

fiennyangeln commented Oct 14, 2018

yangshun left a comment

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Conversation

fiennyangeln commented Oct 8, 2018

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

docusaurus-bot commented Oct 8, 2018 • edited Loading

yangshun 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

endiliey Oct 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

fiennyangeln Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endiliey left a comment • edited Loading

Choose a reason for hiding this comment

JoelMarcey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun commented Oct 11, 2018

fiennyangeln commented Oct 14, 2018

fiennyangeln commented Oct 14, 2018

yangshun left a comment

Choose a reason for hiding this comment

docusaurus-bot commented Oct 8, 2018 •

edited

Loading

endiliey Oct 9, 2018 •

edited

Loading

fiennyangeln Oct 10, 2018 •

edited

Loading

endiliey left a comment •

edited

Loading