Easier handling of multiple pack_tag invocations

tomdracz commented 2 years ago

Continuing from discussions like https://github.com/rails/webpacker/issues/3068

We are now raising an error when pack_tag helpers are used multiple times during the rendering. This solves hard to debug issues with double script loading, but it's fairly heavy handed approach, especially if we don't provide easy way to solve those issues. At the moment, users are required to roll out some custom logic to get all their packs in a single place.

We should look and see if we can improve the experience here in any way. Ideally calling *_pack_tag multiple times would "just" work, but there's complexities here around load order, ideal placement of runtime check etc

As a first step, maybe simply adding a view helper to register additional packs that then get added in pack_tag call is enough to improve the experience?

Suggestions definitely welcome!

tomdracz commented 2 years ago

Note there's also things like https://github.com/shakacode/shakapacker/issues/17#issuecomment-1023088856 that references inability to mix and match defer options.

That might be bit trickier to solve, if you're deduplicating chunks when some use defer and some do not, how do you decide which one can be deferred?

justin808 commented 2 years ago

Here's a solution for distributing calls to the setup of the javascript_pack_tag inside of partials.

The clever part is the content_for in a helper:

Add this helper in /app/helpers/react_helper.rb

module ReactHelper
  def append_javascript_pack(pack_name)
    content_for :view_pack_tags do
      " #{pack_name}"
    end
  end
end

Views then have

<% append_javascript_pack("my-pack") %>

Then the layout has:

  <%
    # Because content_for appends as strings separated by white spaces, pack names are supposed to be space separated too.
    # This way the separation is consistent regardless of their sources.
    pack_tag_args = (yield :view_pack_tags).strip.split(" ")
  %>
  <%= javascript_pack_tag *pack_tag_args unless pack_tag_args.empty? %>

Or

    <%
      # Because content_for appends as strings separated by white spaces, pack names are supposed to be space separated too.
      # This way the separation is consistent regardless of their sources.
      pack_tag_args = (yield :view_pack_tags).strip.split(" ")

      pack_tag_args << "bundle-header" unless skip_header?
      pack_tag_args << "bundle-footer" unless skip_footer?
      pack_tag_args << "bundle-non-production" if Rails.env.development? || Rails.env.staging?
    %>

Or

    <% pack_tag_args = ["homepage", "header"]
       pack_tag_args << "bundle-non-production" if Rails.env.development? || Rails.env.staging?
    %>
    <%= javascript_pack_tag *pack_tag_args %>

Ensure the used layout has a javascript_pack_tag with `(yield :view_tags).strip.split(" ")`

  <%
    # Because content_for appends as strings separated by white spaces, pack names are supposed to be space separated too.
    # This way the separation is consistent regardless of their sources.
    pack_tag_args = (yield :view_pack_tags).strip.split(" ")
    pack_tag_args << "header" unless skip_header?
    pack_tag_args << "footer" unless skip_footer?
    pack_tag_args << "non-production" if Rails.env.development? || Rails.env.staging?
  %>
  <%= javascript_pack_tag *pack_tag_args unless pack_tag_args.empty? %>

There should not be different pack tags importing same modules, specially if React is involved.

In views, add `append_javascript_pack` with the name of the file. If multiple files, make them space separated.

  <%
    app = react_component_hash("App", props: {
      authenticationToken: current_user.authentication_token
    }, prerender: true)
  %>

For SSR, initalize the component rendering with

  <% my_project_app = react_component_hash("MyProjectApp", props: { myId: 1 }, prerender: true) %>

Add any required CSS bundle file and the CSS script from the app variable:

   <% content_for :stylesheet do %>
     <%= app["componentCss"] %>
   <% end %>

Place the app's HTML where you require it:

   <%= app["componentHtml"] %>

jdelStrother commented 2 years ago

Note there's also things like #17 (comment) that references inability to mix and match defer options.

That might be bit trickier to solve, if you're deduplicating chunks when some use defer and some do not, how do you decide which one can be deferred?

We also had problems with the "helpful" error. Some of our pages have a minimal print stylesheet - eg:

<%= stylesheet_pack_tag "application" %>
<%= stylesheet_pack_tag "print", media: "print" %>

which seems like it ought to be allowed. For now we've worked around it by manually setting @stylesheet_pack_tag_loaded = false after the first stylesheet_pack_tag call.

justin808 commented 2 years ago

@jdelStrother any recommended fix?

In the example above, could you have used the "print" media option for both application and print packs?

jdelStrother commented 2 years ago

@jdelStrother any recommended fix?

Nothing good... maybe it should only warn if options isn't present? Or, eg -

  def stylesheet_pack_tag(*names, **options)
-   if @stylesheet_pack_tag_loaded
+   if @stylesheet_pack_tag_loaded == options
      raise "To prevent duplicated chunks on the page, you should call stylesheet_pack_tag only once on the page. " \
      "Please refer to https://github.com/shakacode/shakapacker/blob/master/README.md#usage for the usage guide"
    end

-   @stylesheet_pack_tag_loaded = true
+   @stylesheet_pack_tag_loaded = options

so that it warns you for cases like these:

<%= stylesheet_pack_tag "application", media: "screen" %>
<%= stylesheet_pack_tag "homepage", media: "screen" %>

without impacting cases like these:

<%= stylesheet_pack_tag "application", media: "screen" %>
<%= stylesheet_pack_tag "print", media: "print" %>

which should maybe just be left as completely separate entrypoints.

In the example above, could you have used the "print" media option for both application and print packs?

I'm not sure I follow... you mean like this?

<%= stylesheet_pack_tag "application", media: "print" %>
<%= stylesheet_pack_tag "print", media: "print" %>

That would mean my page has no styles except when viewed on a printer.

I could technically merge them into a single entrypoint and wrap the print styles in @media print { ... }, but it seems unfortunate to break the relatively common pattern of having separate css files for print vs screen when I know that those files don't share any chunks and so shouldn't be prohibited from loading together.

tomdracz commented 2 years ago

Looking back at this, I'm wondering whether we actually need same raise/strictness behaviour with stylesheets. I've originally took the same approach as original webpacker PR that never got merged and ended up applying raise to both helpers.

Need to play around with it to see if any duplication actually happens here, but in general, I wouldn't expect anything here to be as bad as it can be when JS gets re-inited.

Thoughts?

justin808 commented 2 years ago

@tomdracz @jdelStrother While we definitely had terrible issues if JS is reloaded per React hooks, could we consider if the stylesheet pack tag helper should have an option if it should load chunks?

@jdelStrother could you try to figure out what the right webpack configuration looks like and how we'd document this?

Check out:

https://github.com/rails/webpacker/pull/2895

jdelStrother commented 2 years ago

To my knowledge, webpack never splits css into reusable chunks - ie, if you have an entrypoint like application.scss:

@import "bootstrap"
body { color: pink }

it doesn't get split into two vendors-node_modules_bootstrap-abcdef.css and application-abcdef.css chunks.

So, for every CSS entrypoint, it's always only going to consist of a single chunk. So stylesheet_pack_tag(:application, :admin) will always result in two (and only two) style tags. And, if it weren't for the @stylesheet_pack_tag_loaded check, these two forms would produce identical output:

<%= stylesheet_pack_tag :application %>
<%= stylesheet_pack_tag :admin %>

<%= stylesheet_pack_tag :application, :admin %>

which to me suggests the @stylesheet_pack_tag_loaded check should be removed. I may be missing something, though.

justin808 commented 2 years ago

@jdelStrother if you're 💯 sure, can you submit a PR and we'll get this released!

We also need to update the docs!

tomdracz commented 2 years ago

Chunking in css can definitely happen, away from my laptop so don't have example but I've seen it on my app.

Still, double loading CSS should be less problematic than JS case and sometimes desired if you want to use something like media: print

Happy for us to remove the raise for stylesheets

jdelStrother commented 2 years ago

@jdelStrother if you're 💯 sure, can you submit a PR and we'll get this released!

I'm 9️⃣0️⃣ sure at best - have never seen it in my webpack usage, but that's only one app. I'd definitely defer to @tomdracz if he thinks it can happen, though even then I'm struggling to see how it would cause the sort of problems we see if you try and load duplicate chunks via multiple javascript entrypoints.

tomdracz commented 2 years ago

@jdelStrother if you're 💯 sure, can you submit a PR and we'll get this released!

I'm 9️⃣0️⃣ sure at best - have never seen it in my webpack usage, but that's only one app. I'd definitely defer to @tomdracz if he thinks it can happen, though even then I'm struggling to see how it would cause the sort of problems we see if you try and load duplicate chunks via multiple javascript entrypoints.

Exactly, it can happen but the effect is minimal so let's remove the raise from the stylesheet helper. Open up PR @jdelStrother or if you unable to, lemme know and I can set one up over the weekend. Should be pretty straightforward as original only changed two files https://github.com/shakacode/shakapacker/pull/19/files 😉

vtamara commented 2 years ago

Hi. To try out double javascript_pack_tag, I started this repository https://github.com/vtamara/example_multiple_pack_tag

Initially it has commented the second javascript_pack_tag in app/views/layouts/application.html.erb

Is this a good example? Could you please add more situations that should be addressed?

Blessings.

justin808 commented 2 years ago

@vtamara AWESOME to see you try to put together an example repo.

However, the example scenarios should include using Rails partials that need to reference bundles. And we don't want to require that the main layout knows all the scenarios.

Simple view references a bundle and that needs to get reflected in the layout. I think this is what you're referencing.
Using partials in the main view that will reference bundles. My example https://github.com/shakacode/shakapacker/issues/39#issuecomment-1062176162 is designed for this case.
Using partials on the layout, such as for the footer or header. The problem is that the partials might be evaluated too late. Thus, while ugly, we might not be able to support this case.

If you put in some debugging in the evaluation of partials inside from the main layout or from the main view, you'll have a better understanding of the issues I mention.

@tomdracz anything else?

jdelStrother commented 2 years ago

Just to throw in an alternate approach that people might consider - we (Audioboom) have a single Webpack entrypoint for the entire website*, and then let the frontend code deal with dynamically loading any extra bits of functionality via webpack code splitting.

A (oversimplified) calendar-component partial might look something like:

<div data-js-class=Calendar />

And then our entrypoint looks for matching selectors via MutationObservers and/or page-ready events:

watchFor("[data-js-class='Calendar']", () => {
  import("components/calendar")
})

So webpack will break components/calendar.ts into a separate chunk and only load it if necessary. We don't break every component out like this, but anything that is infrequently used or pulls in big dependencies is a good candidate for doing so.

It seems like shakapacker recommends multiple entrypoints for dealing with this sort of thing, but I'm not sure that's a very "webpack approach". (Maybe that's ok! It's a framework for rails users wanting to use webpack, not webpack users wanting to use rails.) But I'm curious what the trade offs are of multiple entrypoints vs codesplitting+dynamic loading.

*: I am oversimplifying things a little here - our iframed embed players do have separate entrypoints, since they share very little code with the main website.

vtamara commented 2 years ago

I improved the example adding a third pack (and React component) ShowTime that just shows the current time.

The file app/vieews/layout/application presents different combinations of loading the packs. https://github.com/vtamara/example_multiple_pack_tag/blob/45add39f294f93744436945307f90bb1006832b7/app/views/layouts/application.html.erb#L9-L27

IMHO the components in the loaded packs should be usable in the layout or wherever needed.

I also think that trying to load twice the same pack should be allowed as long as it keeps the same defer value.

vtamara commented 2 years ago

I have a proposal of solution https://github.com/shakacode/shakapacker/pull/91, that at least allows the example to run as I imagined:

The header and the footer are in one pack (application.js) and are rendered in app/views/layout/application.html. The header is server side rendered, the footer is client side rendered.

The HelloWorld component (that comes from the pack hello-world-bundle.js) is rendered in the view app/views/comments/index.html.erb client side.

The Showtime component (that comes from the pack show-time-bundle.js) is rendered also in the view app/views/comments/index.html.erb client side.

I guess there are situations I didn't cover, so I would appreciate your feedback. Also it is possible that the PR --that is very small-- has mistakes or could be improved, so also please review and audit.

In case it is correct, I think that before merging it, the bug https://github.com/shakacode/shakapacker/issues/88 should be fixed to avoid confusing the cause of that bug with the proposed PR.

tomdracz commented 2 years ago

Looking at the discussion here and trying to get over my initial thinking, I'm struggling to find a good way to get this going.

There are shortcomings of every solution but the closest I can think of is something along the lines of:

Only allow ONE javascript_pack_tag call per page (ideally in the head?)
Provide helpers to append additional tags to the load - those should be able to take options like defer
When javascript_pack_tag is evaluated, we always get everything that needs to be processed. We could then look at the options like defer and deduplicate chunks - if something's declared with and without defer we would prioritise non-deferred load etc

Problem with multiple invocations is that we're using context and load order becomes messy. Calling the helper in the partial might included stuff like runtime chunk, turbo or stimulus in the body rather than head and it will complain. This might cause other issues with Turbo etc.

Having one place to evaluate scripts does make everything bit easier while might be more opinionated. Struggling to figure out a better solution though.

Thoughts?

vtamara commented 2 years ago

Thank you for feedback.

I added to the example another component to show the current date and partials in the layout and in the main template. https://github.com/vtamara/example_multiple_pack_tag/commit/e6ab0f3d7f0f50111dd3432901ae50eb48d54528

(I still have not added turbolinks).

vtamara commented 2 years ago

Debugging how ActionView handles, I think it always process first partials.

Suppossing that and that there is at least one javascript_pack_tag in the layout or in the main template I proposed an improved implementation.

It leaves in a queue the chunks required in the partials.

When it can emit chunks of the main template or of the layout it emits also those in queue.

With this idea, the example works (at least bin/webpacker and after bin/rail s, because I had issues with react_on_rails when running webpack-dev-server)

The way to detect if the javascript_pack_tag was called from a partial or from the main template/layout is not orthodox, but seems reliable to me. The other options I can think of are:

Using Instrumentation of ActiveSupport (but I don't think it would work because there is an event when it enters a partial but not when it exits)
Proposing a patch for ActionView that will allow to have more information in the helpers, i.e to imform what is being processed: a partial or the main template/layout.
Monkey patch ActionView

I would appreciate your reviews: https://github.com/shakacode/shakapacker/pull/91

And if possible please propose a PR for the example including cases that break the logic explained.

tomdracz commented 2 years ago

Just opened https://github.com/shakacode/shakapacker/pull/94 with another stab at improving this. Opinions much welcome!

robotfelix commented 2 years ago

After trying to upgrade to shakapacker from webpacker I ran into a similar problem to https://github.com/shakacode/shakapacker/issues/17#issuecomment-1023088856, although in our case it was wanting to insert some critical JS in the head and put the rest at the end of the body tag (so different places rather than different defer options).

I appreciate I'm coming into this very fresh, but it seems like a relatively simple reworking of the #javascript_pack_tag helper enables my use case without any issues by just tracking what files have been rendered (whether runtime, entrypoint or chunk) and making sure not to render them again within the same request.

module ShakapackerHelper
  def deduplicated_javascript_pack_tag(*names, defer: true, **options)
    @shakapacker_chunks_rendered ||= []
    sources = sources_from_manifest_entrypoints(names, type: :javascript)
    new_sources = sources - @shakapacker_chunks_rendered
    return unless new_sources.present?

    @shakapacker_chunks_rendered.concat new_sources
    javascript_include_tag(*new_sources, **options.tap { |o| o[:defer] = defer }) if new_sources.present?
  end
end

Am I missing something that is very dangerous about this? 🤔

(If there are multiple calls for the same file, with different options (eg: defer), then the first call will win, but that seems reasonable for my use case. Keeping track of the options used for the first render of each file and raising/warning whenever a subsequent call is using different options would make this safer.)

tomdracz commented 2 years ago

Hey @robotfelix The issue here is that you might end up with scripts loaded in unexpected places.

Consider following:

I have a view called index.html.erb
index.html.erb calls a partial called _partial.html.erb
index.html.erb is rended with layout layout.html.erb

Now imagine all three - layout, index.html and partial have javascript_pack_tag call - what will happen?

In short, you will get chunks in weird places. In the above scenario, the layout file will be processed last so by the time you get to it, you've deduplicated chunks and bits like runtime chunk or any chunks referenced by multiple packs get thrown somewhere in the middle of the body. Not particularly great and might cause some issues. (I'm hoping I'm not going mad, as I SWEAR that the partial and everything inside will get processed before the parent)

Your stab at the helper would work I think, as long as you know what the load order would be. If both calls are in the same layout, it will be trivial, the one at the top will get processed first. Then at the second call, you will only get chunks that were not already loaded. Nice and simple. However lack of knowledge of the context where the helper is being called causes massive pain in trying to figure out what can be loaded where.

Your use case is one we haven't seen previously and didn't think about, I guess general thinking in #94 doesn't support it as we're still aiming to just have one place where all the assets live.

justin808 commented 2 years ago

After trying to upgrade to shakapacker from webpacker I ran into a similar problem to https://github.com/shakacode/shakapacker/issues/17#issuecomment-1023088856, although in our case it was wanting to insert some critical JS in the head and put the rest at the end of the body tag (so different places rather than different defer options).

@robotfelix can you give a more specific use case?

josemigallas commented 2 years ago

Hi, we're upgrading our rails app with webpacker to shakapacker and things look good so far. We're using the ReactHelper to append javascript packs from different partials and even though all packs are present in :view_pack_tags, one of them isn't added to the HTML. If I move the append_javascript_pack around (to other partials and views) it eventually gets added to the page and executed. Any tips or ideas?

justin808 commented 2 years ago

@josemigallas I suspect the partial you're having trouble with is on the main layout. The new helper won't work on the layout as it needs to be processed before the layout renders.

justin808 commented 2 years ago

@tomdracz Closing since we merged #94!

josemigallas commented 2 years ago

@justin808 Is this new helper documented somewhere?

tomdracz commented 2 years ago

@josemigallas https://github.com/shakacode/shakapacker#view-helpers info on usage here. Do note that this is only available in v6.3.0 which is still in RC stage. Hopefully we'll be shipping final 6.3.0 soon

josemigallas commented 2 years ago

Just for the record, I put the javascript_pack_tag at the end of every layout and call append_javascript_pack_tag from anywhere in my partials and it works like a charm. Thank you!

damassi commented 10 months ago

@tomdracz - I ran into an issue with this that I can't seem to resolve and was wondering if you might have some strategies.

The issue that @josemigallas raised above got me part of the way towards understanding how things all work, but my case is such to where the partial is loaded entirely asynchronously. Meaning, based on a sequence of events, a modal appears that then triggers the partial. At that point -- and that point only -- the pack script needs to be appended to the DOM and execute.

By going the content_for route I was able to get things partially working, but due to how the code is configured, when the append call is called the js is added (instantly, on page load), then it executes, which means that the node that we need to attach to (for some legacy code) does not yet exist due to the chain of events not yet having been completed.

How would one go about solving this issue?

(Note that i'm migrating from webpacker, where invoking javascript_pack_tag in this way used to work, somehow.)

The ideal is simply: call append_javascript_pack_tag, script tag is inserted, and it doesn't matter when.

Update: I was able to hack around and restore the original behavior by looking in the lib internals and writing an application helper method like

  def append_shakapack_js_pack(pack_name)
    files = current_shakapacker_instance.manifest.lookup_pack_with_chunks!(pack_name.to_s, type: :javascript)

    # Don't include the webpack runtime chunk
    files = files.reject { |file| file.include? 'runtime' }

    capture do
      concat javascript_include_tag(*files)
    end
  end

But this is obviously not ideal.

justin808 commented 10 months ago

@damassi could you configure the JS code to always get installed and be event-driven (triggered by the partial loading). This is how React on Rails works.

If you're open to consulting, please book a time with me, and maybe I can help. Or send me an email. You can read more about how we do consulting here.

damassi commented 10 months ago

Hi @justin808 - the above is mostly just a work-around for a very old and legacy codebase that has many layers; rather than reconfigure (which isn't quite an option) we're trying to preserve backwards compatability with webpacker. Using my hack approach above works but it would be nice to address in shakapack, if possible. I'm wondering what folks think, short of rewriting old code. (I confirmed a moment ago that indeed, the old webpacker approach appends in the manner described, at the time of invocation.)

justin808 commented 10 months ago

The problem is bundle splitting. The default configuration is to use bundle splitting. With bundle splitting, you specify the top-level bundles (entry points) required and Webpack and Shakapacker take care of the magic to give you what you need.

What you seem to want is dynamic code splitting like loadable-components.

Because you're free to use your own Webpack config, that might be a place to consider customizing.

damassi commented 10 months ago

It looks like I was able to (partially) work out a solution here with react-on-rails, however i'm still trying to figure out how to rehydrate once mounted, as right now only the server-side code will execute when prerender: true. But the async issue is solved: said server-side react code will only execute when the partial itself renders and executes in response to a user opening a modal. Without properly being able to rehydrate, however, its quite moot.

In terms of react-on-rails, a simple solution would be to add an async: true prop to the react_component helper which tells the client to use a mutation observer and listen for dynamically-generated server-side-injected dom ID; once found, attach and execute. Because the initial bundle is injected at the very beginning, code splitting, etc, will all work, and it would only be the component lib wrapper pausing to execute until the server-side pass is indeed done rendering and the new dom node appears.

This said, it feels like a bug / oversight that append_javascript_pack_tag will never execute correctly if called within an entirely async context. In webpacker 5 javascript_pack_tag would append, so 6 and above breaks things upon refactor. Maybe its worth updating the docs with info about this? It is quite confusing for the end user and puts a hard stop on migrations to Shakapacker. Calling template partials from things like modals and the like is a fairly common rails pattern, especially in older codebases.

shakacode / shakapacker