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

Use Webpack manifest to associate asset references per pack #6198

Merged
merged 18 commits into from
Apr 15, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 13, 2022

Why: Since our current approach to list assets required per pack is not scalable, and does not account for packs other than document-capture, nor would it support assets deeply referenced via shareable packages (e.g. @18f/identity-components).

What:

  • Implements a Webpack plugin similar to our localization plugin, which scans for calls to getAssetPath
  • Associates those paths with the entrypoint(s) they are included within
  • When those entrypoints are printed, the paths are mapped using Rails asset_path, and assigned to a window global
  • The front-end calls to getAssetPath pull the mapped path from the window global

Testing Instructions:

Ensure there are no regressions to the use of images in the document capture step, e.g. the "warning" icon after a failed submission, or the ID card animated SVG loading interstitial.

webpack.config.js Outdated Show resolved Hide resolved
@aduth aduth changed the title Create manifest to associate asset references per Webpack pack Use Webpack manifest to associate asset references per pack Apr 14, 2022
**Why**: Since our current approach to list assets required per pack is not scalable, and does not account.

changelog: Internal, Build, Generate asset manifest during JavaScript build
Because there's nil-safe navigation, so possible nil values
@aduth aduth marked this pull request as ready for review April 14, 2022 20:50
aduth and others added 2 commits April 15, 2022 09:04
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

**Why**: Avoid risks and complication of dealing with executable script tag
@aduth
Copy link
Member Author

aduth commented Apr 15, 2022

I made a minor adjustment in b970eba to avoid the use of a "full" script tag, leveraging an approach similar to what we've done with locale strings for components (example.

1. Parameter not typed (any)
2. Inferred return value is "string", when it can be undefined
def javascript_assets_tag(*names)
assets = AssetSources.get_assets(*names)
if assets.present?
asset_map = assets.index_with { |path| asset_path(path) }
Copy link
Member Author

Choose a reason for hiding this comment

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

I expect I'll need to revisit this in future work, where we'll want to use this for the design system icon sprite, which can't be loaded across domains, so we'll need to have a way to pass host: asset_host like we do in IconComponent:

asset_path([design_system_asset_path('img/sprite.svg'), '#', icon].join, host: asset_host)

Thinking we might need to have some sort of allowlist, e.g.

Suggested change
asset_map = assets.index_with { |path| asset_path(path) }
asset_map = assets.index_with do |path|
asset_path(path, host: SAME_ORIGIN_ASSETS.include?(path) && asset_host)
end

(host option uses falsey assignment for defaulting)

@aduth aduth merged commit 5a83a82 into main Apr 15, 2022
@aduth aduth deleted the aduth-react-button-icon branch April 15, 2022 18:42
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

2 participants