Confusion between the :to and :under options of pin_all_from

[radiantshaw]

I was testing the pin_all_from method and found what might be a potential issue. pin_all_from requires both the :to and :under options to be passed for this test case to pass:

importmap-rails/test/importmap_test.rb

Lines 50 to 53 in 27236e3

	test "directory pin under custom asset path" do
	assert_match %r|assets/spina/controllers/another_controller-.*\.js|, generate_importmap_json["imports"]["controllers/spina/another_controller"]
	assert_match %r|assets/spina/controllers/deeper/again_controller-.*\.js|, generate_importmap_json["imports"]["controllers/spina/deeper/again_controller"]
	end

This is also apparent by removing the line:

importmap-rails/test/importmap_test.rb

Line 13 in 27236e3

pin_all_from "app/javascript/spina/controllers", under: "controllers/spina", preload: true

from the setup and noticing that the tests still pass. I also verified it by creating a new Rails 7 app and adding both the lines:

importmap-rails/test/importmap_test.rb

Lines 13 to 14 in 27236e3

	pin_all_from "app/javascript/spina/controllers", under: "controllers/spina", preload: true
	pin_all_from "app/javascript/spina/controllers", under: "controllers/spina", to: "spina/controllers", preload: true

to config/importmap.rb and running bin/importmap json. Commenting out the second line doesn't generate any imports but commenting out the first line does.

[radiantshaw]

I think I figured out what's going on. The :under option to pin_all_from is used to find the "path" of the target file (processed file made available inside public/assets via the pipeline):

importmap-rails/lib/importmap/map.rb

Line 136 in 27236e3

[ mapping.path || mapping.under, filename.to_s ].compact.join("/")

But, this only works if the value passed to :under is a suffix of the dir parameter (the first parameter to pin_all_from):

pin_all_from "app/javascript/spina/controllers", under: "spina/controllers"

If the value to :under is different (such as "controllers/spina"), then we have to pass the :to option, which will then be used to find the path instead:

pin_all_from "app/javascript/spina/controllers", under: "controllers/spina", to: "spina/controllers"

In this case, as well, the value passed to the :to parameter has to be a suffix of the dir parameter.

Feel free to correct me if I missed out on anything. If you want me to raise a PR to document this scenario in the README, then I'll be happy to do it.

I still feel that this could be improved so that the pin_all_from method behaves similar to the pin method:

pin_under "controllers/spina", all: "spina/controllers"

This does 2 things:

1. The parameters are accepted in a similar way to pin
2. We don't need the app/javascript prefix

But this obviously includes a change in the public API, so it is up for debate.

Another way can also be to just to modify the pin method to accept an :under argument:

pin "controllers/spina", under: "spina/controllers"

Note that in this case, :under is not accepting "what to pin as" (as seen for the current implementation of pin_all_from), but rather the target path.

I also noticed that this issue is overlapping with #66 so I'm just mentioning that here.