Fix #388 : Displaying date appropriately #559

tarang9211 · 2017-02-20T19:00:38Z

PR for #388

facebook-github-bot · 2017-02-20T19:01:16Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

facebook-github-bot · 2017-02-20T19:09:31Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

gaearon · 2017-02-20T19:30:43Z

This is not fixing the problem. You fixed the example to not use a date as a prop. Instead we need to fix the DevTools to display dates correctly. Once that is fixed, it's fine to revert any changes to test/example. I only suggested adding a date prop there so that you can reproduce the underlying problem.

gaearon

See comment above

tarang9211 · 2017-02-20T19:49:03Z

Oh, im confused. Where should I be making the changes?

gaearon · 2017-02-20T19:56:14Z

DevTools uses a bridge to serialize objects and later deserialize them. Serialization happens in agent/dehydrate.js. So I think you might want to add a special case for Dates there, similarly to how we have a special case for Arrays.

It is probably worth introducing a new type ('date') in addition to 'function', 'object', and 'array'. For a serialized representation, you can probably use date.toJSON(). Then you'd need to find the places in the DevTools UI that look at the data type before displaying it. Based on #459 (which is much bigger, as it adds support for Iterables, but similar in scope), it is possible you’ll need to touch frontend/DataView/previewComplex.js and frontend/PropVal.js.

I hope this helps!

gaearon · 2017-02-20T19:58:00Z

(Note I'm not sure myself. I'd start with adding a special case to dehydrate for dates, and seeing how it works and what breaks.)

tarang9211 · 2017-02-20T20:01:25Z

No worries, I'll dig into this. Should I remove the changes i made above?

gaearon · 2017-02-20T20:04:14Z

You'll want to keep a date prop there so that you can verify your next fixes work. But you should remove JSON.stringify in test since that will prevent you from testing your feature.

tarang9211 · 2017-02-20T20:10:53Z

Got it. Question: why are you referring to it as props when it's in the this.state?

gaearon · 2017-02-20T20:16:47Z

Ah, I missed that. I suggested adding it to props initially so that's what I was referring to. It doesn't matter really—the way you added it to state also works for testing.

tarang9211 · 2017-02-21T14:33:44Z

@gaearon is that a step in the right direction? I put a console log on line 44 of dehydrate.jswhich was basically console.log('reached here') and it never got printed after I create a new to do by opening the index.html in /shells/plains

gaearon · 2017-02-21T16:10:29Z

agent/dehydrate.js

@@ -40,6 +40,13 @@ function dehydrate(data: Object, cleaned: Array<Array<string>>, path?: Array<str
      type: 'function',
    };
  }
+  //check if data is of type date. typeof doesn't support Date, so instanceof is being used
+  if (data instanceof Date) {


I'm not sure this will work across iframes. Let's use data != null && typeof data == 'object' && Object.prototype.toString.call(data) === '[object Date]'. See explanation here. This is what Lodash does, too.

Done. Should I be able to console.log and see where it reaches?

gaearon · 2017-02-21T16:12:06Z

agent/dehydrate.js

@@ -40,6 +40,13 @@ function dehydrate(data: Object, cleaned: Array<Array<string>>, path?: Array<str
      type: 'function',
    };
  }
+  //check if data is of type date. typeof doesn't support Date, so instanceof is being used


Let's remove this comment because it just duplicates the code.

gaearon · 2017-02-21T16:34:09Z

I put a console log on line 44 of dehydrate.js which was basically console.log('reached here') and it never got printed

Here's what I did:

I am running:

./build.sh --watch in shells/plain
../../node_modules/.bin/webpack --watch in test/example (actually it's annoying, can you send another PR to add build.sh there too?)

In the console, I see both logs:

I am also seeing the date in the inspector:

So I think you're going in a right direction, and now need to add code to the frontend (as I mentioned above) to handle the new date type.

tarang9211 · 2017-02-21T17:40:48Z

../../node_modules/.bin/webpack --watch in test/example (actually it's annoying, can you send another PR to add build.sh there too?)

simply copy the build.sh in test/example right?

tarang9211 · 2017-02-21T17:53:12Z

@gaearon Thanks for the suggestion. I got really confused with all the scripts to start the dev environment which is why I could not verify if the code was working. Just pushed a few changes. also added a random color, if you want a specific one let me know. (is hex ok?)

gaearon · 2017-02-21T19:33:48Z

simply copy the build.sh in test/example right?

If that works then yes.

tarang9211 · 2017-02-21T19:38:59Z

Do you want another PR for that?

…

On Wed, Feb 22, 2017 at 1:03 AM Dan Abramov ***@***.***> wrote: simply copy the build.sh in test/example right? If that works then yes. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#559 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFmqGd3TG399n5FCnUjnhnSSxqUv1kA0ks5rezwggaJpZM4MGhaW> .

gaearon · 2017-02-21T19:45:23Z

Yea

tarang9211 · 2017-02-21T20:14:04Z

Sounds good, will do so. Any thoughts on this PR? Now that we're toward the end of this, I can remove the time property from the state right?

jaredly · 2017-02-23T23:38:30Z

I think it's fine to leave time in the state as an example

tarang9211 · 2017-02-24T08:06:49Z

Sounds good! @gaearon Does this look good to you? I'll pull master and create another PR for the build.sh file?

gaearon · 2017-02-24T15:02:29Z

I'm a bit busy this week but I should be able to look at this next week.
Yes, please do.

gaearon · 2017-02-24T22:21:13Z

I am still seeing this:

Am I doing something wrong? How do you test the change? Can you give me a screenshot?

tarang9211 · 2017-02-24T22:33:55Z

I tested this by creating a new to do item and inspecting the todos array.

gaearon · 2017-02-24T22:38:34Z

This is what I’m seeing in the array:

tarang9211 · 2017-02-24T22:39:35Z

This is exactly what I see. Oh one second, is the {...} the problematic part?

gaearon · 2017-02-24T22:44:54Z

Well, the problem is this doesn’t really show Dates specially, it just shows an object instead of them. What we want is to show the date itself—this is why we need to modify the frontend to check for this special type, and display it specially. I see you added the code there, but have you verified it actually runs? Maybe there’s some earlier conditions that prevent it from running.

The end goal is to display Dates same way Chrome displays them:

tarang9211 · 2017-02-24T22:48:05Z

Oh ok. Sorry about that, I'll get to this tomorrow if it's ok. It's 4 15 am where I am. 😄

gaearon · 2017-02-24T22:48:24Z

Yea, no worries. Thank you!

gaearon · 2017-03-02T18:49:46Z

Do you need any help?

tarang9211 · 2017-03-02T19:25:29Z

Hi Dan, sorry for the delay. Got caught up in work, looking at this now.

tarang9211 · 2017-03-02T19:53:21Z

@gaearon is the .toJSON() really needed? that's converting it to a string type.

gaearon · 2017-03-02T19:55:17Z

It's up to you—all I care about is the end result :-). I just suggested one way of producing a string like this out of a date.

tarang9211 · 2017-03-02T19:59:16Z

Alright, so I've been able to debug that it doesn't hit any of the checks in propVal.js and previewComplex.js. When we do this check

  var type = data[consts.type];

it returns object.

Should I modify consts.js?

I'm not sure why the checks are not getting passed.

gaearon · 2017-03-02T20:07:46Z

Would editing it give you anything? I presume it returns object because that's what data says in its type field. The question is, why.

tarang9211 · 2017-03-02T20:12:58Z

Hmm. So data.toJSON() returns a string. It comes into previewComplex.js as this

Object {type: "date", name: "2017-03-02T20:12:15.389Z"}

that's what we're aiming for right?

This is the line it's failing at after debugging:

	  if (!data[consts.type]) {
	    return '{…}';
	  }

gaearon · 2017-03-02T20:18:56Z

Maybe? I don’t know 😄 . You have more context on this code than I do.

gaearon · 2017-03-02T20:19:57Z

All I’m saying is: try to see how other special types (e.g. array) work, and follow the same principle. I don’t know this code well so following my suggestions verbatim is probably not the best idea.

tarang9211 · 2017-03-02T20:26:22Z

Sounds good.

try to see how other special types (e.g. array) work, and follow the same principle.

Could you tell me how to debug arrays, objects?

gaearon · 2017-03-02T20:28:49Z

I'm not sure what you're asking. I think agent/dehydrate.js has examples of treating arrays.

tarang9211 · 2017-03-02T20:31:21Z

Right, I saw that. For example, to test the date formatting I would enter a new todo item in the input box and then view the todos state (and inspect the time property).

Similarly, I was wondering how would I test the object or array? I know it's being handled, but I want to attach a debugger and step through

gaearon · 2017-03-02T20:42:18Z

You can set a breakpoint in those places in dehydrate.js. Since todos are already arrays, I'm not sure what else you could need.

gaearon · 2017-04-20T11:40:30Z

I’m closing since this got stale. Thanks for your effort!

date obj shows up, but not formatted appropriately

2dfe8da

facebook-github-bot added the GH Review: review-needed label Feb 20, 2017

tarang9211 changed the title ~~date obj shows up, but not formatted appropriately~~ Fix #388 : Displaying date appropriately Feb 20, 2017

tarang9211 mentioned this pull request Feb 20, 2017

Show more info for Date objects. #388

Closed

fix: removed regex replacement since it doesn't work

538aa59

facebook-github-bot added the CLA Signed label Feb 20, 2017

gaearon suggested changes Feb 20, 2017

View reviewed changes

fix: added date type to dehydrate

bf2bb92

gaearon reviewed Feb 21, 2017

View reviewed changes

fix: modified logic to check date type, removed extraneous comment

bb0f9aa

fix: handling date type and added styling for date type objects

662e8c5

gaearon closed this Apr 20, 2017

gaearon mentioned this pull request Apr 20, 2017

Set readable names for date objects. #554

Merged

Fix #388 : Displaying date appropriately #559

Fix #388 : Displaying date appropriately #559

Conversation

tarang9211 commented Feb 20, 2017

facebook-github-bot commented Feb 20, 2017

facebook-github-bot commented Feb 20, 2017

gaearon commented Feb 20, 2017

gaearon left a comment

Choose a reason for hiding this comment

tarang9211 commented Feb 20, 2017

gaearon commented Feb 20, 2017 • edited Loading

gaearon commented Feb 20, 2017

tarang9211 commented Feb 20, 2017

gaearon commented Feb 20, 2017

tarang9211 commented Feb 20, 2017

gaearon commented Feb 20, 2017

tarang9211 commented Feb 21, 2017

gaearon Feb 21, 2017

Choose a reason for hiding this comment

tarang9211 Feb 21, 2017

Choose a reason for hiding this comment

gaearon Feb 21, 2017

Choose a reason for hiding this comment

gaearon commented Feb 21, 2017

tarang9211 commented Feb 21, 2017

tarang9211 commented Feb 21, 2017

gaearon commented Feb 21, 2017

tarang9211 commented Feb 21, 2017 via email

gaearon commented Feb 21, 2017 • edited Loading

tarang9211 commented Feb 21, 2017

jaredly commented Feb 23, 2017

tarang9211 commented Feb 24, 2017

gaearon commented Feb 24, 2017

gaearon commented Feb 24, 2017

tarang9211 commented Feb 24, 2017

gaearon commented Feb 24, 2017

tarang9211 commented Feb 24, 2017

gaearon commented Feb 24, 2017

tarang9211 commented Feb 24, 2017

gaearon commented Feb 24, 2017

gaearon commented Mar 2, 2017

tarang9211 commented Mar 2, 2017

tarang9211 commented Mar 2, 2017

gaearon commented Mar 2, 2017

tarang9211 commented Mar 2, 2017 • edited Loading

gaearon commented Mar 2, 2017

tarang9211 commented Mar 2, 2017 • edited Loading

gaearon commented Mar 2, 2017

gaearon commented Mar 2, 2017

tarang9211 commented Mar 2, 2017

gaearon commented Mar 2, 2017

tarang9211 commented Mar 2, 2017

gaearon commented Mar 2, 2017

gaearon commented Apr 20, 2017

gaearon commented Feb 20, 2017 •

edited

Loading

gaearon commented Feb 21, 2017 •

edited

Loading

tarang9211 commented Mar 2, 2017 •

edited

Loading

tarang9211 commented Mar 2, 2017 •

edited

Loading