Explicit nulls on input types using InputValue wrapper

[BenEmdon]

This PR

This is a competing PR with #12. This proposal takes out the explicit null logic in #14.
The aim of this PR is to allow you to set explicit null values on nullable properties on input types.

• Makes required properties non optional
• Removes the old #{propertyName}seen observers triggered on didSet.
• Adds the wrapper type InputValue to GraphQL.swift

New input types are in the form:

class InputType2 {
	// not nullable
	var id: Int
	// not nullable
	var title: String
	// nullable
	var description: InputValue<String>
	
	init(id: Int, title: String, description: InputValue<String> = .undefined) {
		self.id = id
		self.title = title
		self.description = description
	}
	
	// The 
	var serialize: String {
		var string = "{ "
		string += "id: \(id), "
		string += "title: \(title), "
		switch description {
		case .some(let value):
			if let value = value {
				string += "description: \(value), "
			} else {
				string += "description: null, "
			}
		case .undefined: break
		}
		string += " }"
		return string
	}
}

API Breakage

The new change causes some API breakage.

// Before
convenience init(from product: Product) {
  self.init(
    id: product.id, // required
    title: product.title, // nullable
    description: product.description // nullable
  )
}

// After 
convenience init(from product: Product) {
  self.init(
    id: product.id, 
    title: InputValue(orUndefined: product.title),
    description: InputValue(orUndefined: product.description)
  )
}

Closes #12

[dbart01]

The original PR had a deprecation strategy in place to prevent exisiting API breakage. Is it missing here or will it be coming in subsequent PRs?

[BenEmdon]

@dbart01 That is missing in this PR. Let's implement that post #15 and #12

[dbart01]

The generated part of the InputValue switch statement in serialize():

case .defined(let identifier):
if let identifier = identifier {
    fields.append("identifier:\(GraphQL.quoteString(input: identifier))")
} else {
    fields.append("identifier:null")
}
case .undefined: break

Can be simplified it to this:

case .defined(let identifier): fields.append("identifier:\(GraphQL.quoteString(input: identifier ?? "null"))")
case .undefined: break

[BenEmdon]

@dbart01

case .defined(let identifier): fields.append("identifier:\(GraphQL.quoteString(input: identifier ?? "null"))")
case .undefined: break

☝️ This would send the string "null" instead of the value null.

[dbart01]

Right. How about:

case .defined(let identifier): fields.append("identifier:\(identifier == nil ? "null" : GraphQL.quoteString(input: identifier))")
case .undefined: break

A little "busy" but will save a bunch of lines in generated code.

[BenEmdon]

@dbart01

case .defined(let identifier): fields.append("identifier:\(identifier == nil ? "null" : GraphQL.quoteString(input: identifier))")

☝️ That doesn't unwrap the optional though. Of course we could force unwrap after checking if it's nil but why not keep it simple?

[dbart01]

Yeah it was meant to force-unwrap. It save 5 lines of generated code per field. Could be quite a lot depending on number of fields per class.

[dbart01]

It's also important to consider that these breaking changes directly impact Mobile Buy SDK. Can we not just modify #14 to comply with the Ruby style and add functionality for injecting file headers, etc?

cc: @sav007 @davidmuzi @fananta

[dbart01]

Can we discuss here @g-Off ?

[g-Off]

Sure, didn't realize there was an already open discussion going on in GitHub

[dbart01]

#14 has all the changes we made for Buy SDK, which will inevitably benefit Mobile Shopify. It needs to be massaged slightly to be more Ruby-like and add support for file header injection.

[g-Off]

Can we ship this and the rest of the changes in #14 as individual PRs? 14 has multiple independent things lumped together, ideally they are each shipped separately

[dbart01]

If we're okay splitting the commit into individual PRs, then yes. It gets hairy trying to split changes from a single commit.

[dbart01]

Also, it relies on the existing null support in master to be reverted.

[g-Off]

Also, it relies on the existing null support in master to be reverted.

The double optional, correct? If so then I vote we rollback that change anyways and then go from there.

If we're okay splitting the commit into individual PRs, then yes. It gets hairy trying to split changes from a single commit.

I would like to see the independent changes shipped as independent PRs

[g-Off]

Is there a reason not to ship this PR to cover the case of the InputValue type? As far as I can see this is ready to go, does what we want it to do in order to handle the nil/undefined and includes tests

[dylanahsmith]

InputValue didn't exist before, so why add a deprecated initializer?

[dbart01]

That probably shouldn't be there.

[dbart01]

The double optional, correct?

Double optional shouldn't be there.

Is there a reason not to ship this PR

This introduces breaking changes into master. The current implementation of InputValue isn't backwards compatible. #14 introduces InputValue in a backwards-compatible manner.

[dbart01]

There are 4 PRs in place now that add the SDK changes incrementally:

• Revert "Allow manually specifying nil on input objects" [1 of 4] #13
• Internalize [2 of 4] #18
• Docs [3 of 4] #19
• InputValue [4 of 4] #14

@BenEmdon If the Ruby style isn't ideal, at this point it seems much simpler to fix that after it's merged so we can unblock the Mobile Buy SDK. The SDK relies on the changes for the next release, preferably sooner rather than later.

[BenEmdon]

Update

The new InputValue wrapper is defined as:

public enum Input<T> {
	// Serialzeable value
    case value(T?)
	// Not serializable
    case undefined
    
    public init(orUndefined optional: Optional<T>)  {
        if let value = optional {
            self = .value(value)
        } else {
            self = .undefined
        }
    }
}

[BenEmdon]

@dbart01 is there anything stopping this PR anymore?