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

Why can't defaultValue be null? #3

Open
josh-burton opened this issue Jun 24, 2019 · 10 comments
Open

Why can't defaultValue be null? #3

josh-burton opened this issue Jun 24, 2019 · 10 comments

Comments

@josh-burton
Copy link

Is there a reason why defaultValue can't be null?

Providing default values for all preferences is unintuitive ('' for a string instead of null) or for custom types can be quite difficult.

@roughike roughike changed the title defaultValue required? Why can't defaultValue be null? Jun 24, 2019
@roughike
Copy link
Owner

roughike commented Jun 24, 2019

Thanks for the question!

I'll admit that I was a bit on the fence if defaultValue should be nullable or not. If something new that I overlooked arises, I'm open for discussion about changing it.

Why can't a defaultValue be null?

The aim of this library is to make it easy and foolproof to connect persisted values to your widgets. One key drawback in making defaultValue nullable is that you'd have to remember to do null checks in your widgets.

For example, let's pretend that we're storing a String value called nickname:

// Let's pretend that "defaultValue" *can* be null (but it can't):
Preference<String> nickname = preferences.getString('nickname', defaultValue: null);

// ...
Widget build(BuildContext context) {
  return PreferenceBuilder<String>(
    preference: counter,
    builder: (_, String value) => Text(value),
  );
}

The Text widget can't handle a null value - it will crash if the user's nickname is not set.

You could argue that this issue can be easily solved with a null-aware operator, and you'd be right:

  return PreferenceBuilder<String>(
    // ...
    builder: (_, String value) => Text(value ?? ''),
  );

However, if you don't remember to always check the value for null, you'll have crashes that can easily go unnoticed.

(For example, in our app, a nickname is an optional value that the user can fill in later if they want to. My test profile always has a nickname set, but a first time user signing in using Facebook has a null nickname. If they go to the settings page and I forgot to do a null check in the widget, the nickname text field crashes.)

This is why the library forces you to use a non-null defaultValue, so in this case nickname now becomes:

Preference<String> nickname = preferences.getString('nickname', defaultValue: '');

The widget code stays the same, but it will not crash if defaultValue is null. One added benefit of this is that if you're displaying nickname in multiple places, you don't have to duplicate the null checks everywhere in your widgets.

So in a nutshell, the reason for enforcing a non-null defaultValue in a Preference is so that it's always 100% safe to connect a value to your widget as is. There is no need to worry about NPEs.

Another reason is just a "gut feeling™" - it felt weird to emit null values in a Stream. Although "I don't do it because someone smarter than me doesn't do it either" is not really that solid of an argument, RxJava 2 does not allow emitting null values either. Although I haven't really looked into it in detail why that is. Maybe it applies here, maybe it doesn't - if you have input here I'm open for discussion!

What about custom values?

Like you already pointed out, custom values are a special case.

One of the common problems you'll run into is storing a custom value with a JsonAdapter.
Let's pretend we have the following User class:

class User {
  User(this.nickname, this.fullName, this.email);
  final String nickname;
  final String fullName;
  final String email;

  User.fromJson(Map<String, dynamic> json) :
    nickname = json['nickname'],
    fullName = json['fullName'],
    email = json['email'];

  Map<String, dynamic> toJson() => {
    'nickname': nickname,
    'fullName': fullName,
    'email': email,
  };
}

So far so good. Now the problem arises again because we want to do this:

Preference<User> user = preferences.getCustomValue<User>(
  'user',
  defaultValue: null, // Not allowed!
  adapter: JsonAdapter(
    deserializer: (value) => User.fromJson(value),
  ),
);

With primitives, such as Strings and ints, you could just have a default value of '' or 0 - but how do you represent empty User?

One way of doing so would be creating a custom constructor as suggested in the README:

class User {
  // ...
  User.empty() :
    nickname = '',
    fullName = '',
    email = '';
}

and then provide User.empty as the default value.

Admittedly, this is a little cumbersome. With primitives, it's not that bad, but so far these "empty objects" have worked well enough for us that it has not become a problem.

@lucaslcode
Copy link

lucaslcode commented Jun 25, 2019 via email

@roughike
Copy link
Owner

roughike commented Jun 26, 2019

@lucaslcode you're right in that 0 is a "number with a value zero", not "no value set yet". Same would go for other primitives.

I built this library to make the various settings used in our app more manageable. For our use case, it works perfectly right now. We haven't come across a use case yet where we'd need to emit a null value in a Stream. So far, we actually prefer it this way.

However, if you (@athornz / @lucaslcode) have some specific use cases in mind where you'd need to emit null values in a Preference Stream, I would love to see! Just because it works for me / us, doesn't mean it's some complete piece of work that can be put under dome glass on a stool and on display at the museum of perfect Dart packages :-)

@xal
Copy link

xal commented Sep 18, 2019

The best option is to add an additional optional argument 'allowNullDefaultValue' which can be false by default

@josh-burton
Copy link
Author

Yes that would be good. I still think it should be the default behaviour but that would be a good compromise

@roughike
Copy link
Owner

FYI: I'm looking to rethink this during the upcoming weeks, now that NNBD is becoming a thing.

@roughike
Copy link
Owner

I've certainly hit a couple of annoyances with this behavior, although I was able to work around all of them, but I agree that in the end, the developers should be able to choose themselves.

@hacker1024
Copy link

Now that NNBD has been implemented, I think it makes sense to allow null default values. It's no longer possible to accidentally pass null values to widgets that can't handle them.

Btw, shared_preferences's master branch is null-safe, so AFAIK there's nothing blocking this project's migration now.

@pedromassango
Copy link

@roughike any plans to solve this issue? Let me know if I can help (just assign this to me).

@shahmirzali49
Copy link

any progress @roughike ?

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

No branches or pull requests

7 participants