[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[TanaseButcaru]

Github user TanaseButcaru commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62412961
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

I've added an explanation now, but I see that the PR is already merged.
Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cordova-plugin-contacts/pull/125


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62398237
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

code looks good to me, could you please add a comment about the fact that 
when birthday is null, native side(iOS, Android,etc...) will return an 'Invalid 
Date' ? That will make it easier for others to grasp the reason behind this.
After that, we'll be good to go.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#issuecomment-217576715
  
LGTM!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62397911
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

Thanks for the explanation, makes sense.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[TanaseButcaru]

Github user TanaseButcaru commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62396902
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

Probably if a contact has birthday set on device contact list, this does 
not happen.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[TanaseButcaru]

Github user TanaseButcaru commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62396643
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

'Invalid Date' is set by both iOS and Android platorms and that happens 
because contact.birthday is null.
If you run the simple test case I wrote on Jira in the browser console, 
you'll see the behaviour.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[TanaseButcaru]

Github user TanaseButcaru commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62396222
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

The original logic was: try to get new date, if error, do nothing (so leave 
the contact.birthday as it is).
There is no problem with the original code, but it just not catches all 
errors and in this case is the 'Invalid Date' error which never throws.
The error I was getting on iOS was caused by a third party js lib that was 
iterating through contact props, checking if the prop was an object and it was 
then was doing a toJSON() and because contact.birthday was an object but in 
form of an error, that was the key element that made safari throw an error. 
Firefox and Chrome does not throw an error if toJSON() is called on an 'Invalid 
Date' object.

So the error was indirectly caused by contacts plugin.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62395371
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
--- End diff --

:+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62395046
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
+try {
+  contact.birthday = new Date(parseFloat(value));
+  
+  //we might get 'Invalid Date' which does not throw an error
+  //and is an instance of Date.
+  if (isNaN(contact.birthday.getTime())) {
+contact.birthday = null;
--- End diff --

Instead of setting it to null, should an exception be raised instead ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#issuecomment-217565470
  
Do you know why is iOS setting an 'Invalid Date' in the first place ? I 
feel like this fix might end up just masking an issue on the native side.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[TanaseButcaru]

Github user TanaseButcaru commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62394372
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
--- End diff --

I did that null check to skip the part where we enter the try-catch block 
and try to create a new Date. I think this has impact on performance, it's 
faster.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[omefire]

Github user omefire commented on a diff in the pull request:


https://github.com/apache/cordova-plugin-contacts/pull/125#discussion_r62393819
  
--- Diff: www/convertUtils.js ---
@@ -28,10 +28,19 @@ module.exports = {
 */
 toCordovaFormat: function (contact) {
 var value = contact.birthday;
-try {
-  contact.birthday = new Date(parseFloat(value));
-} catch (exception){
-  console.log("Cordova Contact toCordovaFormat error: exception 
creating date.");
+if (value !== null) {
--- End diff --

Why the null check ? If null was returned from the native side, this case 
will be handled by the ```if (isNaN(contact.birthday.getTime()))``` check 
you've got below


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-plugin-contacts pull request: [CB-11223] Better check for ...

[TanaseButcaru]

GitHub user TanaseButcaru opened a pull request:

https://github.com/apache/cordova-plugin-contacts/pull/125

[CB-11223] Better check for date validity of contact.birthday / Fix: 
RangeError Invalid Date



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/TanaseButcaru/cordova-plugin-contacts master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-plugin-contacts/pull/125.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #125


commit 225bfcbaff0bc0d62fd4a557df1761338debcb90
Author: TanaseButcaru 
Date:   2016-05-06T20:08:40Z

handle 'Invalid Date' error




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org