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

refactor(Task): removing gotoTask field #7799

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Jun 24, 2024

What does this PR do?

In the StatefulTask interface we had gotoTask and action, the first one was just a kinda simplification of the second. This PR remove the gotoTask, rename action to rendererAction and deprecate it.

ℹ️ The goal of those are to remove the rendererAction, to have all nicely be done in the main.

Screenshot / video of UI

before
image

After

image

What issues does this PR fix or reference?

Related to #7401

How to test this PR?

  • Tests are covering the bug fix or the new feature

@axel7083 axel7083 requested review from benoitf and a team as code owners June 24, 2024 12:02
@axel7083 axel7083 requested review from dgolovin, jeffmaury and deboer-tim and removed request for a team June 24, 2024 12:02
Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

👍🏼 to the cleanup, but why rename? Especially if you're deprecating I don't know why to bother.

I also don't understand what would come next after the deprecation, as this isn't linked to a specific issue/clear what the replacement would be.

Based on before and after images, my understanding is that you've made this 10s faster. ;-)

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Renaming something that you are deprecating seems opposite

packages/api/src/task.ts Outdated Show resolved Hide resolved
@axel7083
Copy link
Contributor Author

👍🏼 to the cleanup, but why rename? Especially if you're deprecating I don't know why to bother.

The idea was the task can have an action, but not on the renderer side

@jeffmaury
Copy link
Contributor

Code LGTM but deprecation should come once there is an alternative

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 this pull request may close these issues.

None yet

3 participants