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

Allow multiple importmaps in config/importmaps/ #241

Closed
wants to merge 6 commits into from

Conversation

manuelmeurer
Copy link

I implemented the idea mentioned in #240 of allowing multiple importmaps.
It was pretty straightforward and so far works great (for my use-case).

Instead of having one Importmap::Map in Rails.application.importmap, we now have Rails.application.importmaps (plural) that is a hash in the form of:

{
  "application" => #<Importmap::Map>,
  "web" => #<Importmap::Map>,
  "admin" => #<Importmap::Map>,
  "clients" => #<Importmap::Map>
}

config/importmap.rb is now the "application" importmap (which should contain all packages that are needed in all namespaces), config/importmaps/web.rb defines the packages for the "web" namespace, and so forth.

In my app, I can then simply call javascript_importmap_tags :web and it will include the "web" entry point and the correct importmap.

Looking forward to feedback! 😄
And of course happy to work more on the implementation, add tests, etc.

@manuelmeurer manuelmeurer force-pushed the multiple-importmaps branch 3 times, most recently from 1bede97 to bc9792d Compare January 28, 2024 20:37
@manuelmeurer manuelmeurer mentioned this pull request Jan 31, 2024
@@ -10,7 +18,7 @@ def javascript_importmap_tags(entry_point = "application", importmap: Rails.appl

# Generate an inline importmap tag using the passed `importmap_json` JSON string.
# By default, `Rails.application.importmap.to_json(resolver: self)` is used.
def javascript_inline_importmap_tag(importmap_json = Rails.application.importmap.to_json(resolver: self))
def javascript_inline_importmap_tag(importmap_json)
Copy link

Choose a reason for hiding this comment

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

Can you explain why you removed this default value declaration?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I removed it since Rails.application.importmap doesn't exist anymore.
I would even go one step further and pass in an importmap instead of importmap_json as the parameter, similar to what javascript_importmap_module_preload_tags accepts, which would be cleaner IMO.
What do you think?

Copy link

Choose a reason for hiding this comment

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

Yes. Or maybe something like:

Suggested change
def javascript_inline_importmap_tag(importmap_json)
def javascript_inline_importmap_tag(importmap_json)
importmap_json ||= Rails.application.importmaps.find do |importmap|
importmap.name == "application"
end.to_json(resolver: self)

Assuming importmap has an attribute name or something like that for identification.

Copy link
Author

Choose a reason for hiding this comment

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

I went with passing the importmap as a param, since javascript_importmap_module_preload_tags accepts that as well. It looks very clean IMO, what do you think?

Copy link

Choose a reason for hiding this comment

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

I don't have any particular preference. Let's see what others think.

@@ -24,7 +32,7 @@ def javascript_import_module_tag(*module_names)
# Link tags for preloading all modules marked as preload: true in the `importmap`
# (defaults to Rails.application.importmap), such that they'll be fetched
# in advance by browsers supporting this link type (https://caniuse.com/?search=modulepreload).
def javascript_importmap_module_preload_tags(importmap = Rails.application.importmap)
def javascript_importmap_module_preload_tags(importmap)
Copy link

Choose a reason for hiding this comment

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

Can you explain why you removed this default value declaration?

Copy link
Author

Choose a reason for hiding this comment

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

Same reason as with javascript_inline_importmap_tag, there is no single Rails.application.importmap anymore. :)

@manuelmeurer
Copy link
Author

manuelmeurer commented Feb 2, 2024

Another improvement that would make it easier to use javascript_inline_importmap_tag and javascript_importmap_module_preload_tags directly (instead of through javascript_importmap_tags) could be to let both methods accept a param that is either a Importmap::Map or a String, like this:

def javascript_inline_importmap_tag(importmap)
  if importmap.is_a?(String)
    importmap = Rails.application.importmaps.fetch(importmap.to_s)
  end
  importmap_json = importmap.to_json(resolver: self)
  ...
end

Not sure if that is considered a code smell or not though... 😄

@guybedford
Copy link
Contributor

Generally one of the patterns we encourage in JSPM for import map package management is to have a single "main import map" against which individual smaller maps can be "extracted". The reason for this is that the optimal version resolution can be achieved against the main import map between all packages, so that the individual maps then represent slices of that larger map.

JSPM reflects this approach in its inputMap functionality. The main map can be passed into a generation operation via inputMap and then the map to be extracted is provided by a list of module specifiers with the link operation (https://jspm.org/docs/generator/stable/classes/Generator.html#link). In this way, each of the separate import maps can then be fully defined by the combination of the main map and its specifier list.

I'm not sure if an approach like this would suit here or not, but thought I'd share our learnings at least. If there is interest in exploring something like this we could implement a /link endpoint for the JSPM API (while we're exploring /uninstall and /update anyway).

Of course this is just one perspective, and there are many ways to achieve these use cases, happy to discuss further.

@maxim
Copy link

maxim commented Feb 16, 2024

@guybedford This is interesting, made me question whether maps are truly independent in this PR.

Part of the goal here seems to be able to support independent import maps for when you have essentially 2+ apps in one. Like serving admin and non-admin sections. However, I imagine the bin/importmap pin command won't let you pin 2 versions of the same module without manually renaming things. So we should probably go all the way in separating them, including paths where they're stored.

@guybedford
Copy link
Contributor

@maxim import maps do allow multiple versions of the same package using scopes. But yes without utilizing scopes, one does require separate maps.

I have exposed the link api in https://jspm.org/cdn/api#generate-operation to allow extracting a partial map from a larger one through the API.

@manuelmeurer
Copy link
Author

For my specific use case, having one "main import map" from which smaller ones can be extracted does not seem useful.
If you have an admin and a non-admin section, the two of them might not even share a single dependency.

If they do, however, it is of course useful to have the application import map as the one containing all shared dependencies, as implemented in this PR, and all the other ones "inheriting" from the application import map.

@maxim
Copy link

maxim commented Feb 21, 2024

@manuelmeurer Exactly, my use case is just like that — no common dependencies at all. If there were any, it'd be purely by coincidence. Which is why I think it would make this PR more complete if the bin/importmap script was also updated to accept the map name. It could then be used as the dir name, to store and update dependencies separately.

@guybedford
Copy link
Contributor

If you have an admin and a non-admin section, the two of them might not even share a single dependency.

One way to think about this with import maps is similar to dev dependencies in npm. Within a single application, there are parts that use the admin interface and parts that use the main app, which form two separate maps, but if there is cross-over in those dependencies it can be beneficial for them to be part of the same depsolve operation to avoid mismatches where eg one dependency is using one version in the app and another version in the admin system causing issues with interoperability.

Certainly if they are completely distinct "applications", then separate import maps make sense, one has to make the decision here on a case by case basis.

But for a standard MPA, there likely is a lot of dependency crossover where sharing the depsolve can be useful.

@maxim
Copy link

maxim commented Feb 25, 2024

@guybedford Yeah it makes perfect sense, I agree. I think you're describing various degrees of distinction within a single application, that are all covered by main map, extracted maps, and scopes. And like you said, I think this PR addresses the situation (which easily occurs in wide diversity of Rails apps out there) where 2 essentially distinct applications just happened to be "hosted" inside one codebase. An argument could be made that these should indeed be 2 separate Rails apps, one mounted into another. 🤔

@manuelmeurer
Copy link
Author

An argument could be made that these should indeed be 2 separate Rails apps, one mounted into another. 🤔

I've never seen that in reality, though, and I suspect it would add a lot more complexity than simply having two namespaces (e.g., admin and web) under which to scope the controllers, views etc.

I don't really perceive the different parts of my app (I have admin, web and clients) as distinct applications, just separate UIs for separate user groups, but working with the same application logic and structure.

I'll try to fix the bin/importmap script now to accept an identifier.

@seanabrahams
Copy link

What is needed to get this merged?

We're in the "need a separate importmap for our admin area" bucket and this seems like the right solution.

@manuelmeurer
Copy link
Author

The commands now all work with an (optional) "importmap" argument, and all tests pass locally.
@sarmad90 I'd appreciate another review! 😄

…te3 (>= 2.0), already activated sqlite3-1.7" error
@manuelmeurer
Copy link
Author

With Rails main and Sprockets, test/installer_test.rb fails because app/assets/config/manifest.js is not created when running bin/rails importmap:install.
Not sure if that is cause by the changes in this PR?

@manuelmeurer
Copy link
Author

manuelmeurer commented Sep 9, 2024

Ok, so I think there is another good reason to implement multiple importmaps: gems can easily overwrite existing packages and entry points when they add their own stuff to the app's importmap.

I added https://github.com/rails/mission_control-jobs/ to my app and after that my Stimulus controllers didn't work anymore.
Turns out mission_control-jobs uses

pin_all_from ..., under: "controllers", to: "mission_control/jobs/controllers"

which overwrites my app's

pin "controllers", to: "controllers/index.js"

I changed mission_control-jobs to use its own importmap and now everything works again!

@dhh
Copy link
Member

dhh commented Sep 15, 2024

I found #253 to be a nicer way to deal with this issue. Please confirm that's sufficient to fix the problem for you.

Also, we should fix mission_control-jobs to rely on this.

@dhh dhh closed this Sep 16, 2024
@manuelmeurer
Copy link
Author

@dhh #253 is definitely simpler and solves the issue that only the relevant entry points should be preloaded.
However, all modules are always included in <script type="importmap">, regardless of whether they are used in a specific part of the app or not.

If we expand the example from the docs with a few admin modules:

# config/importmap.rb
pin "@github/hotkey", to: "@github--hotkey.js", preload: 'application'
pin "md5", preload: ['application', 'alternate']
pin "adminscript1", preload: 'admin'
pin "adminscript2", preload: 'admin'
pin "adminscript3", preload: 'admin'

and then include

<%= javascript_importmap_tags 'application' %>

it will output

<script type="importmap" data-turbo-track="reload" nonce="Qlxh230F92bk+V5nd5kPMw==">{
  "imports": {
    "@github/hotkey": "https://myapp/assets/@github--hotkey-XXX.js",
    "md5": "https://myapp/assets/md5-XXX.js",
    "adminscript1": "https://myapp/assets/adminscript1-XXX.js",
    "adminscript2": "https://myapp/assets/adminscript2-XXX.js",
    "adminscript3": "https://myapp/assets/adminscript3-XXX.js"
  }
}</script>

<link rel="modulepreload" href="/assets/javascript/@github--hotkey-XXX.js">
<link rel="modulepreload" href="/assets/javascript/md5-XXX.js">

So the three adminscript modules are always included in the output, even if they are never used in the "application" part of the app.
Depending on how many different parts the app has, these could potentially be dozens of modules.

Especially with modules that are only meant to be loaded in the admin part of an app, it might not be desirable to have them visible in the HTML source of all the other parts of the app.

Having separate importmaps would solve this and only render the modules that are actually used in each part (which could be preloaded or not preloaded as well).
But of course this would bring more complexity to the project, so the simpler approach of #253 might be preferable.

@jfi
Copy link

jfi commented Sep 17, 2024

@manuelmeurer It looks like the approach you could take is to name your "application" modules something else, e.g. "frontend", and then do:

<%= javascript_importmap_tags 'frontend' %>
<%= javascript_importmap_tags 'admin' %>

I'm going to give this a try today using this approach, where the admin modules are inserted from an Engine.

This then still allows application to include everything by default?

@manuelmeurer
Copy link
Author

@jfi If I understand it correctly, all modules are always included in <script type="importmap">, no matter which entry point is used. Correct me if I'm wrong.

@aseroff
Copy link
Contributor

aseroff commented Sep 17, 2024

@jfi If I understand it correctly, all modules are always included in <script type="importmap">, no matter which entry point is used. Correct me if I'm wrong.

As author of other PR, yes, that is correct. My approach was focused only on reducing unnecessary preloads, but the concern about exposing other modules is valid.

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.

8 participants