Rename Task.available_tasks to .load_all and deprecate

[davidstosik]

Task.available_tasks not only returns all descendants of the Task class, it also requires all files in the application that may be defining such class.

available_tasks does not carry explicit meaning of how heavy the operation could be, so this renames it to load_all, which should be better at communicating that, and deprecates the old name.

I'll have to adjust this PR once #877 is merged.

[etiennebarrie]

I'm not in favor of this. I prefer the name to be a noun than a verb. We can add documentation instead.
I also don't think it's correct to say that it requires or loads classes, Zeitwerk does that.

[davidstosik]

@etiennebarrie Thank you for your comment.

I disagree with your statement. The whole purpose of defining this method instead of calling descendants directly is to make sure that all Ruby files defining a task have been required. (Using const_get is just a trick to force Zeitwerk to require them if they haven't been already.) Also note that it calls a method named load_constants.

A noun is exactly what lacks meaning of a possibly heavy task, in my opinion. Developers calling Task.available_tasks might expect the list of tasks to be just there, ready to be looked at, whereas in reality calling that method will sometimes trigger the requirement of multiple files (more than 600 in Shopify Core's case, for example), and every time it'll trigger a recursive process of looking into all constant names under the Maintenance namespace.

I'm afraid relying on documentation alone would likely not reach most developers.

I also assumed that I had obtained agreement to do this before opening this PR, here.
Happy to discuss and study alternatives though.

---

My real opinion on this topic though, is that a "recursive eager constant loader" should be a separate tool (it could even be its own gem, or a Zeitwerk plugin), and maintenance-tasks should call something like load_all_constants_under(Maintenance) ; Task.descendants in the two places where it needs to display a list of all tasks.

[adrianna-chang-shopify]

This change makes sense to me, although let's discuss further if you still have reservations @etiennebarrie .

While I understand your perspective on the noun vs verb stance, noun (#available_tasks) to me implies a simple getter, and the case @davidstosik is making is that this method is often not a simple getter and can be computationally expensive. The fact that Zeitwerk does this under the hood is unimportant -- saying that the method "loads" classes is appropriate IMHO because it forces Zeitwerk to load constants that aren't yet loaded.

While I agree that we should reach for changes in the documentation ahead of changing the API, I support David's point that the current method name of #available_tasks fails to convey the computational side of the method.

[davidstosik]

Thank you @adrianna-chang-shopify!
I'll merge my PR now, but please let me know if there's anything you'd like me to improve on this. 🙏🏻

[etiennebarrie]

The way I see this, available_tasks isn't really computation-heavy.

Quick thought experiment, we could define it in terms of

class MaintenanceTasks::Task
  def self.available_tasks
    descendants
  end
end

And in that case, I don't expect a verb but really a name.

And only in development, for developer experience reasons, cause the method to get the constants (causing Ruby to autoload based on Zeitwerk's setup):

class MaintenanceTasks::Engine < ::Rails::Engine
  initializer "maintenance_tasks.autoload" do
    next unless Rails.env.development?
    autoload_tasks = Module.new do
      def available_tasks
        load_constants
        super
      end
    private
      def load_constants
        # elided
      end
    end
    MaintenanceTasks::Task.singleton_class.prepend(autoload_tasks)
  end
end

Since in production, getting constants that have already been autoloaded is not costly, for simplicity we didn't split the logic.

---

BTW we've previously told users some features we didn't want to build in could be handled by overriding available_tasks (e.g. #788) which is now going to break without a deprecation. The contract was that we would call that method, and we're not anymore (actually also somewhat problematic for #866). I guess asking users to patch the gem was always a bit shady to begin with, hopefully these people have good test coverage… 😬

I feel like #866 solved the main problem we were having that testing one task end-to-end (i.e. using a Run and job) would load them all and could be slow. As far as I know, this change is fixing a problem we never had.

[adrianna-chang-shopify]

And in that case, I don't expect a verb but really a name.

Right, but the reality is that the method does load constants in dev / test envs (for developer exp reasons, but that was a decision we made). To me, this change is simply about signalling to users that this method loads constants and can thus be slow, which should hopefully prompt users to think before reaching for it in a test or in their own code.

BTW we've previously told users some features we didn't want to build in could be handled by overriding available_tasks

I didn't like telling people to patch it 😂 And anyways, we didn't suggest it in any official capacity, so I'm not too worried about breaking that contract. I did grok our internal repos, and the only repo overriding it currently is Core.