Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix JSON.parse error (shared NSUserDefaults) #5

Merged
merged 2 commits into from
May 24, 2016

Conversation

MacKentoch
Copy link
Contributor

When NSUserDefaults is shared (via group) with a widget for instance it can lead to JSON.parse error when value stored by widget is a JS primitive: string or number.

Fix here userDefaults.get and userDefaults.set for

  • string
  • number
// in swift, your widget could store :
let userDefaults = NSUserDefaults(suiteName: "SUITE_NAME")
userDefaults?.setObject("a string for this key", forKey:"KEY_NAME")
userDefaults?.synchronize()

this fix insure string and number won't throw errors.

**When `NSUserDefaults` is shared (*via group*) with a widget for instance it can lead to JSON.parse error when value stored by widget is a JS primitive: string or number.**

Fix here `userDefaults.get` and `userDefaults.set` for 
- string
- number

```objective-c
// in swift
let userDefaults = NSUserDefaults(suiteName: "SUITE_NAME")
userDefaults?.setObject("a string for this key", forKey:"KEY_NAME")
userDefaults?.synchronize()
```
@wwayne
Copy link
Owner

wwayne commented May 22, 2016

@MacKentoch Thanks, I'm not familiar with iOS development. Does this change compatible with normal use case? I mean for those who are using NSUserDefault directly, looks NSUserDefault cant set Integer withsetObject`.

@MacKentoch
Copy link
Contributor Author

MacKentoch commented May 23, 2016

@wwayne Thank you too you helped me since I did not have to reinvent the wheel and I prefer your react-native-user-default for being simple.

I use react-native-user-default in an iOS project where I migrate an existing application all witten in Swift to react native.

I have a today widget that uses NSUserDefault store to share data (simple strings) with the main application.

I apply this fix so that my today widget (that remains in Swift) don't have to be rewritten at all.
Otherwise it was broken since I received a

JSON.stringify('test') = '"test"' when I was waiting for "test".

To be honest I did not use number (or NSNumber equivalent).
But it should be ok since setObject can set

  • NSString (string: covered and tested since it was my need)
  • NSDate (date: I did not cover)
  • NSNumber (number: covered but not tested since I did not use it in my project)
  • ...

A further explanation — an extract from [https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSUserDefaults_Class/#//apple_ref/occ/instm/NSUserDefaults/setObject:forKey:] — on setObject in apple documentation :
Declaration
SWIFT

func setObject(_ value: AnyObject?,
        forKey defaultName: String)

OBJECTIVE-C

- (void)setObject:(id)value
           forKey:(NSString *)defaultName

Parameters
value
The object to store in the defaults database.

defaultName
The key with which to associate with the value.

Discussion
The value parameter can be only property list objects: NSData, NSString, NSNumber, NSDate, NSArray, or NSDictionary. For NSArray and NSDictionary objects, their contents must be property list objects. See What is a Property List? in Property List Programming Guide.

To finish

Let me add a suggestion.
I better use promises than callbacks and since I had an eye on the JS code I saw I could use like (I use):

  userDefaults
  .get(_KEY_,_GOUP_)
  .then(
    data => dispatch(anAction(data))
  ).catch(
    err => {
      dispatch(anErrorReport(err));
    }
  );

I guess it would be nice adding as an alternative use case on README.
But it is yours so choice is yours.

@wwayne
Copy link
Owner

wwayne commented May 24, 2016

@MacKentoch thanks for your detailed explanation 👍 , merging, I will release new version to NPM soon.

@wwayne wwayne merged commit 2840687 into wwayne:master May 24, 2016
@MacKentoch
Copy link
Contributor Author

@wwayne thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants