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

Change return type of WorkerInterface::getWorkflows() and WorkerInterface::getActivities() #491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nyholm
Copy link
Contributor

@Nyholm Nyholm commented Aug 23, 2024

I would like to change the return type of WorkerInterface::getWorkflows() and WorkerInterface::getActivities(). They were both introduced in #14 and as far as I can see it was not publicly discussed if it should be iterable or RepositoryInterface.

What was changed

To change the return type is a hard BC break which we want to avoid. That is why this PR introduce a new interface which is is compatible with WorkerInterface and will narrow down the return type. This interface will of course be removed before we release next major version.

As you can see, the Worker is already compatible with this change.

Why?

If one would like to add activities to the worker one currently have limited options. There is no way to "standardize" activity behaviour for your application.

In my case, I want to automatically add MethodRetry to each activity in my application with some "my app"-specific standards.

My current workaround is of course not to use WorkerInterface and just type hint to Worker.

Copy link

vercel bot commented Aug 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
php ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 23, 2024 9:53pm

@roxblnfk roxblnfk added the Feature New feature or request label Aug 23, 2024
@roxblnfk roxblnfk assigned roxblnfk and unassigned roxblnfk Aug 23, 2024
@roxblnfk roxblnfk self-requested a review August 23, 2024 22:15
@Nyholm
Copy link
Contributor Author

Nyholm commented Aug 24, 2024

The tests failures seams unrelated

Copy link
Collaborator

@roxblnfk roxblnfk left a comment

Choose a reason for hiding this comment

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

Hello. I'm afraid I cannot accept this change for the following reasons:

  • WorkerInterface::getWorkflows() and WorkerInterface::getActivities() seem designed to return collections without the ability to modify them directly within SDK internals through add() or remove() methods.
  • The fact that Worker returns RepositoryInterface is an internal implementation detail.
  • The RepositoryInterface is in an internal namespace. Considering The SDK does not use TaskQueue when searching for the Workflow to be started. #481, there is a chance that something might change around the Worker class and the RepositoryInterface.

As I see it, you have a need for certain functionality. Instead of exposing internal details, I suggest creating an Issue where we can discuss the implementation.

@wolfy-j
Copy link
Collaborator

wolfy-j commented Sep 3, 2024

@Nyholm, do you want to jump on a call with us? This way, we can discuss our current limitations and approach to changes like that. We are happy to see these PRs and really grateful to you, but this is a massive SDK, and we are trying to be careful not to leak internal implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants