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

Proposal to facilitate multiple import maps #69

Closed
wants to merge 4 commits into from

Conversation

zzeligg
Copy link

@zzeligg zzeligg commented Nov 14, 2021

Being in the process of upgrading and modernizing a Rails application, I plan to use import maps. However, my app has 3 definite scopes which each have their own functionality and layout (public, admin and the actual application scope, where authenticated users perform their activities).

A single 'application' import map shared between scopes and layouts does not cut it for my application (for performance reasons and for eventual security concerns).

My proposal is therefore to add such functionality to importmap-rails.

This PR is rather light and represents the minimum changeset to this gem that allow to use it with more than a single import map (i.e. these changes would allow to use the helpers as is).

The changes are to the helpers module, simply adding the ImportMap::Map instance as a parameter in 2 of its methods. The added parameter value defaults to Rails.application.importmap, therefore it does not break the existing usage, but does allow to use the helpers methods with different ImportMap::Map instances (instead of having to rewrite complete helpers in the application that reproduce the same thing with another importmap instance).

That being said, as it is, one still has to go against the grain when defining multiple import maps instances, for example:

  • how should multiple config/importmap.rb files be named and where should they exist?
  • how to deal with app.config.importmap.paths for multiple import maps?

If there is interest for more functionality to be added to importmap-rails to support multiple import maps, I will gladly contribute towards it. Otherwise, at least the changes in this PR make it still possible to use the gem without having to redefine custom helpers in the application.

@radiantshaw
Copy link
Contributor

Hello @zzeligg. I'm not an active contributor to the repo but still thought of suggesting an alternate idea. In the hope to keep only one importmap.rb file across the whole project, I'd say we can do something like:

# config/importmap.rb

scope :admin do
  pin "admin", to: "admin.js"
  # ... other pins for the `:admin` scope ...
end

scope :public do
  pin "public", to: "public.js"
  # ... other pins for the `:public` scope ...
end

# The `:default` argument can also be omitted and it will be assumed
scope :default do
  pin "application", to: "application.js"
  # ... other pins for the `:default` scope ...
end

And then inside each of the view layout files, we can do:

<%# views/layouts/admin.erb %>

<%= javascript_importmap_tags "admin", scope: :admin %>
<%# views/layouts/public.erb %>

<%= javascript_importmap_tags "public", scope: :public %>
<%# views/layouts/public.erb %>

<%# Takes the `entry_point` as `"application"` and `:scope` as `:default` if not specified %>
<%= javascript_importmap_tags %>

The only problem is that the Import Map spec also has a "scopes" property along with the "imports" property, so the method name scope can be confusing. An alternate name might be namespace.

@zzeligg
Copy link
Author

zzeligg commented Nov 14, 2021

Thanks @radiantshaw, that's actually a good idea. If interest for out-of-the-box support for multiple import maps is intended, this would make for good specs. My intent for now was to have the bare minimum to achieve it.

And as to avoid the possible confusion with the scope nomenclature, it could be simply be referred to as map:

# config/importmap.rb

# outside an explicit `map` block, declarations are tied to a default 'application' scope.
pin "application", to: "application.js"

#  specifying a map block creates a separate ImportMap::Map instance
map :public do
  pin "public", to: "public.js"
  ...
end 

It could be implemented by creating a class which encapsulates a collection of ImportMap::Map instances, adding that map method to the DSL and adjusting the view helpers parameters accordingly to render one map or another.

@zzeligg
Copy link
Author

zzeligg commented Nov 16, 2021

I went ahead and implemented it.

So, basically, the use case of having a single importmap is pretty much preserved, except for the name of the importmap instance in the application, which I renamed to Rails.application.importmaps (plural). That variable is a collection of Importmap::Map which has a default map accessible as Rails.application.importmaps.default. So existing apps which refer to this instance for some reason would need to change it.

I have also added an attribute, Importmap::Map#entry_point, which defaults to 'application'. The entry_point will be used (unless overridden) when calling javascript_importmap_tags helper.

The helpers can still be called with default parameters, in which case they will render the default importmap, but a map name can be given such as javascript_importmap_tags(map: 'public')

I have added tests to make sure the default usage remains intact (with no specific map scope) and to make sure the right importmaps are used when present.

So basically, from a usage standpoint, not much has changed. You can define a specific map like so:

# config/importmap.rb

# outside an explicit `map` block, declarations are tied to a default 'application' scope.
pin "application", to: "application.js"

#  specifying a map block creates a separate ImportMap::Map instance
map :public, entry_point: 'public' do
  pin "public", to: "public.js"
  ...
end 


test "importmaps instance has an specific map" do
h = generate_importmap_json('public')
assert_equal h['imports'].keys, [ 'public', 'md5', 'controllers/hello_controller', 'controllers', 'helpers/requests' ]
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand going through the code and this line, this doesn't include the "application" part. I feel we should do this instead:

pin "whatever" # This should be included globally instead of only inside the `:default` map

map :public do
  pin "more_whatever" # This should be only included in the `:public` map
end

Basically, the above should generate:

{
  "imports": {
    "whatever": "/assets/whatever.js",
    "more_whatever": "/assets/more_whatever.js"
  }
}

for the :public Import Map, and:

{
  "imports": {
    "whatever": "/assets/whatever.js"
  }
}

for the :default Import Map.

This will allow not having to repeat a pin/pin_all_from if something should be included everywhere. Thoughts?

@xdmx
Copy link

xdmx commented Dec 18, 2021

This would be a big plus for me as well. I have various pages where I only need some of the imported libraries and not all of them, and that would help on reducing the extra data that is added to the html on each response with the list of all import maps, especially because some of those libraries depend on others, and thus it's a bit long list of useless data

@jmckible
Copy link

Thank you for putting this together @zzeligg. We've also got an application with distinct scopes and this is a must-have feature in order to adopt importmap.

After implementing this in our application, I'd agree with @radiantshaw that including the default pins in all named maps helps clean up the definitions. For us, it makes sense to name all maps and not use the default scope independently.

I've organized our maps into app/javascript/maps. The only other DSL clean up I'd suggest is marking the entry_point on the pin definition, like so:

map :app  do
  pin 'maps/app', entry_point: true, preload: true
end

@dhh
Copy link
Member

dhh commented Jan 17, 2022

I think there's something interesting here, but I'm not convinced it's worth it. I just hit this case in HEY where we needed two substantially different entry points, and my first instinct was "well we need separate import maps!". But an import map is basically just a load path. You don't have to use all of it!

So in my case, I ended up just mapping those extra pins I needed for the alternate case in the global import map, and then having that separate entry point that only referred to a small subset of the pins.

The one drawback of this approach is that the preloads may differ, but that seems like something we could probably solve without introducing the complexity of separate blocks in the import map.

@radiantshaw
Copy link
Contributor

@dhh What if you have a situation where you're using a lot of files, but for a different entry point, you only need a very minor subset? In that case, we'd be shipping load paths that might never get used.

I know that a couple of bytes is not a big deal, but it might become one if there are tons of load paths.

Just my two cents, but feel free to disagree if you feel it's not what the project wants.

The one drawback of this approach is that the preloads may differ, but that seems like something we could probably solve without introducing the complexity of separate blocks in the import map.

Maybe we can think of this PR as a solution to this problem; the side effect of which also includes not shipping unused load paths? And not the other way around?

@dhh
Copy link
Member

dhh commented Jan 17, 2022

That was exactly my case. Our import map is probably 150 files. For this different entry point, I just needed 3. The 147 useless lines were just such a small part of the overall picture that it wasn't worth addressing.

But yeah, I can see the issue with the preload setup. In that case, I'm almost thinking we could have a way of passing in a preload list, rather than codifying it as part of the importmap.

@xdmx
Copy link

xdmx commented Jan 17, 2022

@dhh what's interesting about reading Hey's source code is that the sign in page is ~40K, of which ~30.3K is just the importmap/preload part.

I'm worried that for projects with many Javascript dependencies/code it'd be something that would prevent people to use it (or at least having them thinking deep about it). Especially when you have totally different scopes and users (eg. buyers, sellers, platform admins)

Turbo and the browser would only parse that once on the first page load, but it's still data that would need to be sent through the wire (Turbo Frames and Streams excluded)

@dhh
Copy link
Member

dhh commented Jan 17, 2022

That's intentional since we get to setup all the libraries while someone is typing in their user name and password. We definitely could have made a slimmer entry point if we didn't want to preload as much, though. Anyway, that weight is all from loading actual libraries! So it's about the preloads, not the import map.

So I'd love to explore a different way to do the preloads. Maybe it's just an array passed into the helper tag to setting up?

@dhh
Copy link
Member

dhh commented Feb 19, 2022

Going to close this in favor of an open invitation to explore a way to handle different preloads rather than hardcoding them in the import map.

@dhh dhh closed this Feb 19, 2022
@zzeligg zzeligg deleted the multiple-importmaps branch May 12, 2022 14:13
@zzeligg zzeligg restored the multiple-importmaps branch May 12, 2022 14:13
@benbonnet
Copy link

benbonnet commented Jul 8, 2023

just to understand the situation, is the entrypoint strictly bound to app/javascript/application.js ?

Ended up here as I was looking for a different path for my entrypoint (ex. app/frontend/app.js instead of app/javascript/application.js). No success so far

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.

None yet

6 participants