Skip to content

Conversation

@zach-klippenstein
Copy link
Collaborator

Making these types non-nullable doesn't add any inherent value or safety. There may be workflows
that want to define nullable types, so we should let them. Workflows that define their types to be non-nullable still get all the null safety they expect.

Making the rendering type non-nullable doesn't add any inherent value or safety. There may be workflows
that want to define a nullable rendering type, so we should let them. Workflows that define their rendering
types as non-nullable still get all the null safety they expect.
Making the input type non-nullable doesn't add any inherent value or safety. There may be workflows
that want to define a nullable input type, so we should let them. Workflows that define their input
types to be non-nullable still get all the null safety they expect.
Making the state type non-nullable doesn't add any inherent value or safety. There may be workflows
that want to define a nullable state type, so we should let them. Workflows that define their state
types to be non-nullable still get all the null safety they expect.
Copy link

@kirillzh kirillzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

@rjrjr
Copy link
Contributor

rjrjr commented May 8, 2019

Was there a specific issue that drove this change? And if nullability is okay now, why isn't it okay for OutputT?

@zach-klippenstein
Copy link
Collaborator Author

zach-klippenstein commented May 8, 2019

Was there a specific issue that drove this change?

I was working on the legacy integration, and it would have been convenient to use null to represent the lack of a value. In general, I would expect a whole host of workflows to have an "optional" state or rendering instead of making a custom union type just for that.

if nullability is okay now, why isn't it okay for OutputT?

OutputT is the only type in the set that represents an "event" instead of a declarative value. There might be a use case for null, but in general "the lack of a value" for outputs is represented by just not emitting any.

Also, null is currently used to indicate "no output was emitted" in WorkflowHost.Update, and while we can certainly change that to support null output values if necessary, it's a more involved change that these others so I wanted to avoid it until there was a clear need.

@zach-klippenstein zach-klippenstein merged commit 62a98c7 into master May 8, 2019
@zach-klippenstein zach-klippenstein deleted the zachklipp/remove-any-bounds branch May 8, 2019 19:06
@zach-klippenstein zach-klippenstein added the kotlin Affects the Kotlin library. label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kotlin Affects the Kotlin library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants