Skip to content

Opt in to Webpack(er) chunk splitting#4553

Merged
aduth merged 9 commits intomasterfrom
aduth-webpack-split-chunks
Jan 14, 2021
Merged

Opt in to Webpack(er) chunk splitting#4553
aduth merged 9 commits intomasterfrom
aduth-webpack-split-chunks

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 4, 2021

Why:

  • Since it is the default behavior as of the upcoming Webpacker 6+ release, it will make a future upgrade easier.
  • It will allow for reuse of large shared dependencies (e.g. using React outside of the document capture pack without bundling a new copy of React into each pack where it is used).
    • This effort was prompted by a desire to use React outside the document capture screen (specifically to reuse the existing Alert React component)
  • Conceptually well-aligned to javascript_pack_tag_once, facilitating eventual migration to render all applicable scripts once at the bottom of each page.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Contributor Author

aduth commented Jan 5, 2021

A thought occurs to me: As they describe in their "old" (current) documentation, it's recommended to output all chunks at once, to best deduplicate shared groups of chunks.

Important: Pass all your pack names when using this helper otherwise you will get duplicated chunks on the page.

This could work well with javascript_pack_tag_once, so we could update all instances to use that instead. This reduces the risk of duplicate chunks. The main caveat is to make sure that the scripts would work well when output at the bottom of the page rather than inlined to where they're called. I don't expect it'd be a problem though.

@mitchellhenke
Copy link
Contributor

That sounds good to me 👍🏼

@jmhooper
Copy link
Contributor

jmhooper commented Jan 5, 2021

Nice, this is super nifty!

The main caveat is to make sure that the scripts would work well when output at the bottom of the page rather than inlined to where they're called.

Yeah, I wouldn't expect that to be a problem either.

@aduth
Copy link
Contributor Author

aduth commented Jan 5, 2021

Started looking at shifting everything to use javascript_pack_tag_once. It seems mostly straightforward, but there's a few edge cases:

  • The session timeout scripts assume a reference to window.LoginGov.Modal, which is not available if application script is deferred.
    • Fix is as simple as moving 'til after the subsequent render_javascript_pack_once_tags
  • The shared "failure" view allows a presenter to render inline JavaScript, and there's at least one (max_attempts_reached_presenter.rb) that makes a similar reference to window.LoginGov.
    • Not sure yet how best to fix this. Will dig in tomorrow.

@aduth
Copy link
Contributor Author

aduth commented Jan 6, 2021

Started looking at shifting everything to use javascript_pack_tag_once.

3a96209

The session timeout scripts assume a reference to window.LoginGov.Modal, which is not available if application script is deferred.

  • Fix is as simple as moving 'til after the subsequent render_javascript_pack_once_tags

ae495a4

The shared "failure" view allows a presenter to render inline JavaScript, and there's at least one (max_attempts_reached_presenter.rb) that makes a similar reference to window.LoginGov.

  • Not sure yet how best to fix this. Will dig in tomorrow.

e046e1a, deferring execution until DOMContentLoaded. Tested and appears to work fine.

@aduth
Copy link
Contributor Author

aduth commented Jan 6, 2021

Another interesting edge case: Some packs rely on a specific order of scripts being loaded, e.g. personal-key-page-controller expected to occur after application so that it has access to window.LoginGov.Modal synchronously on script load:

const modal = new window.LoginGov.Modal({ el: modalSelector });

Ideally we'd have stronger dependencies defined, using imports. Another option similar to e046e1a would be to wait 'til page load to call the constructor.

I'd also thought maybe it'd be as easy as moving javascript_packs_tag_once back to the head. However, the order of where javascript_packs_tag_once is called in the markup is not always reliable. Because partials are rendered before its layout (as far as I can tell), the javascript_packs_tag_once for the application pack would always be called last.

Next best option I considered for a solution is to allow the method to receive an option to control the order that the pack is enqueued (a prepend option). See 67ef74469 and dc2273dd6.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM some small nits

aduth and others added 9 commits January 14, 2021 08:55
**Why**:

- Since it is the default behavior as of the upcoming Webpacker 6+ release, it will make a future upgrade easier.
  - See: https://github.com/rails/webpacker/blob/1aca8aa/6_0_upgrade.md
- It will allow for reuse of large shared dependencies (e.g. using React outside of the document capture pack without bundling a new copy of React into each pack where it is used).
  - This effort was prompted by a desire to use React outside the document capture screen (specifically to reuse the existing Alert React component)
- Conceptually well-aligned to javascript_pack_tag_once, facilitating eventual migration to render all scripts once at the bottom of each page.
  - See: https://web.dev/render-blocking-resources/
**Why**: Split chunks will output many separate scripts, but there should be at most one with name starting with each chunk. Others will be prefixed either `runtime~` or `vendors~`.
**Why**: Avoid issue where using javascript_packs_with_chunks_tag multiple times inlined may cause duplicate chunks tags to be rendered.
**Why**: Session timeout makes synchronous reference to LoginGov window globals which are only assigned by packs
**Why**: This JavaScript is inlined prior to pack output. As such, LoginGov global is not yet assigned. Waiting until page has fully loaded provides assurance that those scripts will have then been loaded.
**Why**: More granular control over order of script enqueuing, in cases such as layouts where layout template would be rendered after partial, and scripts which occur _before_ the partial in markup would still enqueue scripts _after_ the partial.
**Why**: See 67ef74469
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth force-pushed the aduth-webpack-split-chunks branch from d1d1263 to aea5b1c Compare January 14, 2021 13:59
@aduth aduth merged commit f40bba7 into master Jan 14, 2021
@aduth aduth deleted the aduth-webpack-split-chunks branch January 14, 2021 14:20
aduth added a commit that referenced this pull request Jan 15, 2021
**Why:** So that users are redirected automatically when completing SAML authentication.

In #4553, all JavaScript packs were changed to use the `javascript_packs_tag_once` helper, which is intended to be used in rendering all JavaScript chunks once at the bottom of the page. However, this must be complemented by a call to the `render_javascript_pack_once_tags` helper. This exists in the base template, but since the `saml_post_binding` view doesn't inherit from this template, the helper is never called, and therefore the script is never added to the page.

Since we don't need to worry as much about deduplicating script chunks on this page, we can simply call directly to Webpacker's `javascript_packs_with_chunks_tag` helper.
aduth added a commit that referenced this pull request Jan 15, 2021
**Why:** So that users are redirected automatically when completing SAML authentication.

In #4553, all JavaScript packs were changed to use the `javascript_packs_tag_once` helper, which is intended to be used in rendering all JavaScript chunks once at the bottom of the page. However, this must be complemented by a call to the `render_javascript_pack_once_tags` helper. This exists in the base template, but since the `saml_post_binding` view doesn't inherit from this template, the helper is never called, and therefore the script is never added to the page.

Since we don't need to worry as much about deduplicating script chunks on this page, we can simply call directly to Webpacker's `javascript_packs_with_chunks_tag` helper.
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.

4 participants