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

Separate importmap from the application #163

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

henrikbjorn
Copy link
Contributor

This is based upon the importmap usage seen in ActiveAdmin.

This gem essentially provides a complete Application and as such it makes sense to have the importmap separate from the application it is embedded into.

There is a 0% chance of the javascript included here to be used outside of the gem itself.

It is still possible to extend the importmap if that is needed.

MissionControl::Jobs.importmap.draw do
  pin "something"
end

This will prevent the application assets from polluting the host apps
assets.
@henrikbjorn
Copy link
Contributor Author

Fixes #132

Possibly fixes #140

@henrikbjorn
Copy link
Contributor Author

@rosa What do you think about this?

@henrikbjorn henrikbjorn changed the title Encapsulate Importmap into the Gem itself Separate importmap from the application Oct 7, 2024
@manuelmeurer
Copy link

I think this might be fixed by #175.

@henrikbjorn
Copy link
Contributor Author

henrikbjorn commented Oct 16, 2024

I think this might be fixed by #175.

It is not. The assets will still be part of you normal application included in the importmap.

There is no reason that they should be, since MissionControl::Jobs are an "embedded application" into your main one.

@manuelmeurer
Copy link

Ah ok, got it. How/where would you store the assets then?

With #175, the assets are still included in the importmap, but only loaded (and preloaded) in the Mission Control views.

@henrikbjorn
Copy link
Contributor Author

Ah ok, got it. How/where would you store the assets then?

With #175, the assets are still included in the importmap, but only loaded (and preloaded) in the Mission Control views.

The same way, they are just part of a separate importmap which is configured like shown in the PR description.

Same way as ActiveAdmin.

Then it wont show up in your applications own importmap and maybe break things.

@manuelmeurer
Copy link

Oh ok, I finally understood what this PR does. 😆
It's a better solution than what I did in #175, so let's go with this (if @rosa agrees).

@rosa
Copy link
Member

rosa commented Oct 30, 2024

Sorry for the delay here! I've been busy with other stuff and neglecting this gem, but I'm back now and in full focus, so I'll be catching up with all PRs and issues. This looks great to me, going to do a couple of tests to be completely sure, but it makes perfect sense. Thank you so much, @henrikbjorn 🙏

@@ -9,7 +9,7 @@
<meta name="turbo-cache-control" content="no-cache">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bulma@1.0.1/css/bulma.min.css">
<%= stylesheet_link_tag "mission_control/jobs/application", "data-turbo-track": "reload" %>
<%= javascript_importmap_tags "application-mcj" %>
<%= javascript_importmap_tags "application", importmap: MissionControl::Jobs.importmap %>
Copy link
Member

Choose a reason for hiding this comment

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

I believe passing a custom importmap here (rails/importmap-rails#187) wasn't available from version < 2, perhaps with this change, we should require that version 🤔 Like:

diff --git a/Gemfile.lock b/Gemfile.lock
index 185a970..5560422 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -29,7 +29,7 @@ PATH
   remote: .
   specs:
     mission_control-jobs (0.3.3)
-      importmap-rails
+      importmap-rails (~> 2.0)
       irb (~> 1.13)
       rails (>= 7.1)
       stimulus-rails
diff --git a/mission_control-jobs.gemspec b/mission_control-jobs.gemspec
index fc03152..07240ba 100644
--- a/mission_control-jobs.gemspec
+++ b/mission_control-jobs.gemspec
@@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
   end

   spec.add_dependency "rails", ">= 7.1"
-  spec.add_dependency "importmap-rails"
+  spec.add_dependency "importmap-rails", "~> 2.0"
   spec.add_dependency "turbo-rails"
   spec.add_dependency "stimulus-rails"
   spec.add_dependency "irb", "~> 1.13"

Copy link
Member

Choose a reason for hiding this comment

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

No, sorry, I don't know how I looked this up 😅 It was added in version 1.2.1, which is kind of old, but perhaps it's worth making that explicit for older apps 🤔

diff --git a/mission_control-jobs.gemspec b/mission_control-jobs.gemspec
index fc03152..76e081e 100644
--- a/mission_control-jobs.gemspec
+++ b/mission_control-jobs.gemspec
@@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
   end

   spec.add_dependency "rails", ">= 7.1"
-  spec.add_dependency "importmap-rails"
+  spec.add_dependency "importmap-rails", ">= 1.2.1"
   spec.add_dependency "turbo-rails"
   spec.add_dependency "stimulus-rails"
   spec.add_dependency "irb", "~> 1.13"

@@ -9,7 +9,7 @@
<meta name="turbo-cache-control" content="no-cache">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bulma@1.0.1/css/bulma.min.css">
<%= stylesheet_link_tag "mission_control/jobs/application", "data-turbo-track": "reload" %>
<%= javascript_importmap_tags "application-mcj" %>
<%= javascript_importmap_tags "application", importmap: MissionControl::Jobs.importmap %>
Copy link
Member

@rosa rosa Oct 30, 2024

Choose a reason for hiding this comment

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

I believe passing a custom importmap here (rails/importmap-rails#187) wasn't available from version < 2, perhaps with this change, we should require that version 🤔 Like:

diff --git a/Gemfile.lock b/Gemfile.lock
index 185a970..5560422 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -29,7 +29,7 @@ PATH
   remote: .
   specs:
     mission_control-jobs (0.3.3)
-      importmap-rails
+      importmap-rails (~> 2.0)
       irb (~> 1.13)
       rails (>= 7.1)
       stimulus-rails
diff --git a/mission_control-jobs.gemspec b/mission_control-jobs.gemspec
index fc03152..07240ba 100644
--- a/mission_control-jobs.gemspec
+++ b/mission_control-jobs.gemspec
@@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
   end

   spec.add_dependency "rails", ">= 7.1"
-  spec.add_dependency "importmap-rails"
+  spec.add_dependency "importmap-rails", "~> 2.0"
   spec.add_dependency "turbo-rails"
   spec.add_dependency "stimulus-rails"
   spec.add_dependency "irb", "~> 1.13"

@rosa rosa merged commit 78ee499 into rails:main Oct 30, 2024
5 checks passed
rosa added a commit that referenced this pull request Oct 30, 2024
Follow up to #163, which uses support for passing a custom importmap to
the `javascript_importmap_tags` helper.
@henrikbjorn henrikbjorn deleted the importmap-changes branch October 31, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants