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

Additional stimulus controller "lookup" paths in importmap_helper.rb #38

Closed
forsbergplustwo opened this issue Jan 28, 2021 · 3 comments
Closed

Comments

@forsbergplustwo
Copy link
Contributor

forsbergplustwo commented Jan 28, 2021

Firstly, thanks for this library.. it feels like the future! It's also been a great intro to ESM and how that works.

We're running into an issue using this together with a Rails engine, because the importmap_helper.rbfile is hardcoded to look in the Rails.root folder only, when resolving the paths for stimulus controllers.

  def importmap_list_from(*paths)
    Array(paths).flat_map do |path|
      if (absolute_path = Rails.root.join(path)).exist?
        find_javascript_files_in_tree(absolute_path).collect do |filename|
          module_filename = filename.relative_path_from(absolute_path)
          module_name     = importmap_module_name_from(module_filename)
          module_path     = asset_path(absolute_path.basename.join(module_filename))

          %("#{module_name}": "#{module_path}")
        end
      end
    end.compact.join(",\n")
  end

if (absolute_path = Rails.root.join(path)).exist?

We can work around this by manually adding our engine's stimulus controllers to the importmap.json file like so:

{
  "imports": {
    "turbo": "<%= asset_path "turbo" %>",
    <%= importmap_list_with_stimulus_from "app/assets/javascripts/controllers", "app/assets/javascripts/libraries" %>,
    "slideshow_controller": "<%= asset_path "slideshow_controller" %>"
  }
}

where slideshow_controller resides under the engine.

It would be great if we could specify which root paths to use in this helper, so we can include AppKit::Engine.root in the helper path, for example and have it work automatically.

Would this be a welcome option if we gave it a go for a PR? A bit unsure how best to do it, while also keeping the interface simple like it is now: importmap_list_from(*paths) so open to ideas and suggestions.

@dhh
Copy link
Member

dhh commented Jan 28, 2021

I think we could make this work by having it accept both relative and absolute paths. Relative would work as now, going off Rails.root, but absolute (starting with /) would allow for engines 👍

@forsbergplustwo
Copy link
Contributor Author

Thanks, that's a great suggestion. I'll give it a shot 😎

@forsbergplustwo
Copy link
Contributor Author

PR #39 created to add support for this.

The change could have been just taking this line:

      if (absolute_path = Rails.root.join(path)).exist?

And making it:

     if (absolute_path = Rails.root.join(Pathname.new(path))).exist?

But it felt like too much magic and hard to understand. Hence I've added a clearer method. Happy to change if you feel example above is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants