Add a preload_pack_asset helper

[guillaumebriday]

Cf: #2125 and #2125 (comment)

[jakeNiemiec]

Thanks for the PR, I have a few concerns. Is there a reason why preload is preferred to prefetch or preconnect? I am under the impression that preload is for priority loading things like fonts & workers that will be required on the user's current page. The rails docs also uses a web worker as an example.

Depending on where javascript_packs_with_chunks_preload_tag is put it can negatively effect browser scheduling. This entirely negates the usefulness of lazy loading and progressive web enhancement.

I also worry that the user will assume that their scripts will be run.

This is what I would change:

• support preload and prefetch
• convey poor preload adoption https://caniuse.com/#search=preload
• focus on preload specific files like fonts or images instead every chunk in a pack
• convey how webpack has it's own methods of preload and prefetching: https://medium.com/webpack/link-rel-prefetch-preload-in-webpack-51a52358f84c

[guillaumebriday]

Thank you for the feedback !

I was not aware that rails provides the preload_link_tag, I changed my PR to use this instead but it's only available in rails 5.2.

In the other hand, I don't know we can preload only images and fonts with Webpacker, if you have any ideas ?

I read your comment in the related issue and I got your point. Maybe we could add a helper just for fonts and images, and let Webpacker handle only JS ?

[jakeNiemiec]

but it's only available in rails 5.2.

I'll let @gauravtiwari chime in on this. I am not sure about that, I don't want to break things for some users.

Maybe we could add a helper just for fonts and images, and let Webpacker handle only JS ?

Agreed. I think the best course of action is to create a preload_pack_asset helper. It will need to resolve the file assets outside of the entrypoints object in manifest.json. Example from one of my projects:

  <%= preload_pack_asset 'fa-regular-400.woff2' %>
  <%# turns into something like %>
  <link href="/packs/fa-regular-400.d8O6LxGN.woff2" rel="preload" as="font" type="font/woff2" crossorigin>

What do you think?

[jakeNiemiec]

@guillaumebriday We may be able to get away with something like this: #2125 (comment)

[guillaumebriday]

@jakeNiemiec I ended up doing a very basic wrapper for preload_link_tag. It's still only available on Rails >= 5.2 :/

I think we should insist in the documentation on what you said in this comment #1874 (comment)

What do you think ?

[guillaumebriday]

Hey,

I added a condition to check if Rails version is greater than 5.2.x.

The tests are now green, what do you think ?

Thank you !

cc @jakeNiemiec @gauravtiwari

changes addressed

[jakeNiemiec]

I would need to defer to @gauravtiwari, I don't have too much experience with Rails versions inter-op. I would raise an error/warning when that if statement is false though.

[gauravtiwari]

Thanks @guillaumebriday @jakeNiemiec

How about we define a link tag if one isn't available? So pre-5.2 we define a link tag by hand and set the preload option otherwise use the one provided by 5.2+

Something like this:

if self.class.method_defined?(:preload_link_tag)
  preload_link_tag(current_webpacker_instance.manifest.lookup!(name), options)
else 
  # define_tag
  # https://github.com/rails/rails/blob/b9ca94caea2ca6a6cc09abaffaad67b447134079/actionview/lib/action_view/helpers/asset_tag_helper.rb#L268
end

[guillaumebriday]

Thank you @gauravtiwari for the idea !

self.class.method_defined?(:preload_link_tag) works very well !

However, it's not that easy if it's not defined and I'm facing multiple issues with < 5.2 versions.

For example:
Template::Types is not available in Webpacker.
So I tried with Mime::Type.lookup_by_extension(extname).try(:to_s) but it does not work with, at least, woff2, I don't know why.

The method resolve_link_as is a private method in Rails and I'm not sure it's a good idea to duplicate it in Webpacker.

At this point I think the best solution is to made it available only for Rails > 5.2 with a clear warning in the documentation.

Why do you think @gauravtiwari @jakeNiemiec ?

Thank you

[gauravtiwari]

Thanks @guillaumebriday for your efforts, really appreciate it. What do you think of failing in the else clause so a user knows that tag isn't supported. Something like, "you need rails > 5.2 to use this tag".

Will also clarify this in changelog.

[guillaumebriday]

@gauravtiwari Yes I think it's a pretty reasonable compromise !

I pushed it if you want to check again.

Thanks

[gauravtiwari]

Thanks @guillaumebriday 🍰

[guillaumebriday]

Thanks @gauravtiwari @jakeNiemiec for the feedbacks !