Prevent adding existing autoload_paths

[thutterer]

What are you trying to accomplish?

This prevents the set_autoload_paths initializer to add paths into ActiveSupport::Dependencies.autoload_paths that are already in there.

This is likely a non-issue for most projects, and ActiveSupport handles this, I assume. But I ran into this in a codebase where the ActiveSupport::Dependencies.autoload_paths array is already frozen when the view_component.set_autoload_paths initializer does its thing.
With this change, it is possible to manually add the preview paths to the autoload paths like this:

    config.autoload_paths.push("#{config.root}/spec/components/previews")
    config.view_component.preview_paths << "#{config.root}/spec/components/previews"

The set_autoload_paths initializer then has no paths_to_add and doesn't touch the (maybe frozen) ActiveSupport::Dependencies.autoload_paths.

What approach did you choose and why?

I first thought Array's union method would be the perfect replacement for concat but it's not as it returns a new array instead of making the change on the original. That's why I now chose a more verbose approach with a variable for the diff. That should also make it easy to read and understand the code.

Anything you want to highlight for special attention from reviewers?

I couldn't find a good place to add/update a spec for this. Please advise if/where to do that. Thanks!

[joelhawksley]

@thutterer thanks for taking the time to propose this PR. I'm curious to see the sandbox application in test updated to reproduce and thus necessitate this change. Mind giving that a shot?

[thutterer]

Hi @joelhawksley

I've tried a couple of things over the last few days, but haven't yet found a good way to test the initializer code without seriously messing with the sandbox application.

The reason why this is tricky is that this code change is only required when zeitwerk gets loaded before the initializers run. See here for more information why one might do that, but basically:

A side-effect of this is that in the initializers, config.autoload_paths is already frozen.

AFAIU, I'd either need a whole new sandbox just for this test, or change the sandbox in a way that affects all tests, which is not what I want either.

As an alternative approach I've tried to move the code of the initializer into a method. I thought I then can at least unit-test the method, but when minitest is ready, zeitwerk already froze the autoload path, even in the current default setup.

I had a look through the test suite though, and custom preview paths are already thoroughly tested and these tests didn't break with my change.

So would you consider merging this PR without additional tests?

[joelhawksley]

@thutterer thanks for looking into writing tests for this change. I'm feeling torn here so perhaps I can share some more thoughts and you can let me know what you think.

First and foremost, we try really hard on this project to make sure that any change we introduce cannot be unintentionally reverted. This allows us to more readily accept community contributions knowing that if CI passes, we won't be breaking any intended behaviors. This approach has generally served us well.

Second, I'm wondering why this has yet to come up until now. My gut says it might be due to an unusual application configuration on your end?

In this case I do believe that adding another test application would be the way forward. It will help us identify if the use case is something we want to support in the framework and will add a backstop from this change regressing in the future.

[thutterer]

@joelhawksley Thank you for taking the time to look into this and share you thoughts. Much appreciated, especially as this is fixing such a specific corner case situation.

First and foremost, we try really hard on this project to make sure that any change we introduce cannot be unintentionally reverted. This allows us to more readily accept community contributions knowing that if CI passes, we won't be breaking any intended behaviors. This approach has generally served us well.

100% agree with this approach.

Second, I'm wondering why this has yet to come up until now. My gut says it might be due to an unusual application configuration on your end?

It is definitely not the Rails default what we do here, but I don't know how unusual it really is. Let me explain:

Our app started long before Zeitwerk replaced the old autoloading mechanics in Rails.
With Zeitwerk, one breaking change for us was that Rails loads Zeitwerk after the initializers in config/initializers are loaded. Autoloading before Zeitwerk is loaded was now deprecated.

But at that point we already had a lot of autoloaded constants in our initializers, so we had to move the loading of Zeitwerk earlier than these initializers.

But a side-effect of this is that in the initializers, config.autoload_paths is already frozen.

I'd bet some other older Rails apps faced the same challenge and might have solved it the same way we did. They would all have the same issue. Probably they just haven't started to use ViewComponent previews yet 😉

In this case I do believe that adding another test application would be the way forward. It will help us identify if the use case is something we want to support in the framework and will add a backstop from this change regressing in the future.

OK, I will try that, but it will take me a couple of days to get to it. Also, I'm a bit concerned that the test setup for this change will be literally 100x more (in LOC and complexity) than the change itself, making the "not worth the added project complexity for such a corner case" argument to not merge this even stronger.

So would it maybe sufficient to explain the change in a code comment? I know it's not ideal, but my reasoning here is that the change is so self-contained in the initializer code that it's close to impossible to break it without touching the code itself, where a comment then would make it clear why this approach was taken. What do you think?

[joelhawksley]

@thutterer thank you for your thorough reply ❤️. I'd be happy to merge this with a comment if you can add a regression test as a fast follow, but only if this is a blocker for you. I'm about to travel for holiday and Brighton Ruby so my responses will be delayed until July 😄

[thutterer]

@joelhawksley Turns out I was totally overthinking this... no special setup required to test this new behavior.

I've added a simple regression test now. Please have a look whenever it's convenient, no rush. Have fun in 🇬🇧!

[joelhawksley]

Very nice! Thank you for your patience in getting this landed ❤️