Skip to content

Gives WorkflowAction access to props. #81

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 29, 2020
Merged

Gives WorkflowAction access to props. #81

merged 1 commit into from
Jun 29, 2020

Conversation

rjrjr
Copy link
Contributor

@rjrjr rjrjr commented Jun 29, 2020

Closes #56.

@rjrjr rjrjr requested a review from zach-klippenstein June 29, 2020 18:03
@rjrjr rjrjr requested a review from a team as a code owner June 29, 2020 18:03
@rjrjr rjrjr force-pushed the rjrjr/props-to-actions branch from 0a939ed to 935265b Compare June 29, 2020 18:20
rjrjr
rjrjr commented Jun 29, 2020
View reviewed changes
workflow-core/src/main/java/com/squareup/workflow/WorkflowAction.kt
* Convenience function that returns a [WorkflowAction] that will just set the state to [newState]
* (without considering the current state) and optionally emit an output.
* Convenience function that returns a [WorkflowAction] that will just set the state to
* [newState] (without considering the current state) and optionally emit an output.
*/
@Deprecated(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zach-klippenstein We should delete these, they're so dangerous. Is that tracked?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so. We don't have any issues for removing deprecated APIs afaik.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the only ones I really care about. After this merges I'll put up a PR to snip them. Not worth tracking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought they were still extremely popular.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are, but they're also often extremely a bad idea, and misused more often than not. And easy enough to recreate on your own w/o us sanctioning them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#85

zach-klippenstein
zach-klippenstein approved these changes Jun 29, 2020
View reviewed changes
Copy link
Collaborator

@zach-klippenstein zach-klippenstein left a comment

Choose a reason for hiding this comment

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

The only reason that the render context needs to be aware of the props type is so that type inference can infer the type of actions when sent to action sinks and stuff, right?

@rjrjr
Copy link
Contributor Author

rjrjr commented Jun 29, 2020

The only reason that the render context needs to be aware of the props type is so that type inference can infer the type of actions when sent to action sinks and stuff, right?

And for the actions tied to any worker or workflow output.

@rjrjr rjrjr merged commit 12bfb12 into main Jun 29, 2020
@rjrjr rjrjr deleted the rjrjr/props-to-actions branch June 29, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zach-klippenstein zach-klippenstein zach-klippenstein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pass props to WorkflowActions
2 participants
@rjrjr @zach-klippenstein