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

Figure out optional dictionary fields #188

Closed
tarikeshaq opened this issue Jul 31, 2020 · 4 comments · Fixed by #316
Closed

Figure out optional dictionary fields #188

tarikeshaq opened this issue Jul 31, 2020 · 4 comments · Fixed by #316
Assignees

Comments

@tarikeshaq
Copy link
Contributor

tarikeshaq commented Jul 31, 2020

Based on the discussion in mozilla/application-services#3440

We need a clear story on how we represent optional fields in dictionaries.

A few things from that discussion:

  • We may need to set default values
  • Should they be nullable?

I Haven't looked at the details here, but opening this for further discussion in this repo

┆Issue is synchronized with this Jira Task

@jhugman
Copy link
Contributor

jhugman commented Aug 7, 2020

Thinking by documenting here:

  • Values with nullable types are allowed to be null. They most closely correspond to T? in Kotlin and Swift, or Option<T> in Rust.
  • optional is allowing the fields or argument to be omitted, and the behavior when it is. For JS, the key is simply not there in the object. For Swift and Kotlin, the keys are there, but with default values. Rust has no such concept.

I think what I'm proposing here is to use optional to use foreign language features to fully construct the struct before crossing the FFI.

E.g. in Swift, a struct generates a memberwise initializer that accounts for default values.

struct TVCharacter {
    let name: String
    let yearsActive: Int = 0
}

let roslin = TVCharacter(name: "Laura Roslin")
let adama = TVCharacter(name: "William Adama", yearsActive: 45)

Above, yearsActive is optional, but not nullable. Below, yearsActive is nullable, but not optional.

struct TVCharacter {
    let name: String
    let yearsActive: Int?
}

let starbuck = TVCharacter(name: "Kara Thrace", yearsActive: null)

Once the struct is fully constructed with all the values set in Swift, only then is it sent over the FFI.

The only confusion in my mind is this: what happens when we specify a field that is optional, but don't provide a default value:

  • fail fast! we can't omit the key, so we need a default value.
  • guess! Essentially make the Default::default().
  • silently make them nullable/Option<T> types, and set them to null/None.

I don't know which of these are the least bad.

@linacambridge @rfk @tarikeshaq ? thoughts?

@rfk
Copy link
Collaborator

rfk commented Aug 7, 2020

This makes sense to me, thanks for writing it out so clearly. I favour this option in the short term:

fail fast! we can't omit the key, so we need a default value.

If only because it's easy to change in a backwards-compatible way later.

I also think it's worth taking a step back and asking ourselves: this requirement comes from mozilla/application-services#3440 where we have a dictionary with a tonne of optional fields, but is "optional fields" really the abstraction we want here, or do we want the equivalent of a HashMap?

(I personally feel like having an enumerated set of known fields is probably the right thing, but I don't know much about how it will be used by the underlying rust code, and if we're just going to match on its fields as if it were a HashMap then...)

@tarikeshaq
Copy link
Contributor Author

For

but I don't know much about how it will be used by the underlying rust code, and if we're just going to match on its fields as if it were a HashMap then

The AppContext is meant to be used to figure out if a user is targeted by an experiment or not, and will be matched against an expression language statement from the server. A hashmap definitely works, but I'd rather have it as a set of known values, to make it more explicit which fields we support (and possibly easier to change fields in the future without forgetting something).

Now that I think about it, the fields on AppContext would be better represented as nullable as opposed to optional. Since I wouldn't want them to exist if the application didn't specify them.

For

fail fast! we can't omit the key, so we need a default value.

I like this as well! I'm guessing we'd specify default values on the IDL?

@jhugman
Copy link
Contributor

jhugman commented Sep 28, 2020

#283 does the hard work of implementing literals.

This should be relatively simple now: most of the work here is in testing in test_rondpoint.

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 a pull request may close this issue.

4 participants