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

Rewrite for 2.1 and esbuild-based hanami-assets-js package #120

Merged
merged 77 commits into from
Sep 7, 2023

Conversation

jodosha
Copy link
Member

@jodosha jodosha commented Mar 2, 2022

2.1

This is a complete rewrite and simplification of the code base.
We're now leveraging Esbuild.

Usage

To see the new way of using this gem, please refer to the updated README.

Changes

Please refer to CHANGELOG.

Acknowledgements

Special thanks to @KonnorRogers for listening and helping during the process. 🙏 🌸

@jodosha jodosha self-assigned this Mar 2, 2022
jodosha added 3 commits March 2, 2022 10:24
This reverts commit dc4bc79.

Move `ravsamhq/notify-slack-action@v1` at the bottom
jodosha and others added 4 commits July 26, 2023 11:15
This allows us to extract the manifest from Configuration, and let Configuration remain a much simpler object responsible for holding the settings only, and not returning any “active” components of Hanami::Assets.

I’m not 100% happy with the name “Source”, but I consider this a reasonable next step as we figure out the best structure for this gem.
This can represent a single asset and provide a rich set of methods for it. Start with methods for the path, url, and subresource_integrity_value.

Return asset instances from Source#[], and correspondingly remove the separate Source#asset_path and #subresource_integrity_value methods.

There’s still a naming tension yet to be resolved here, in that Helpers#path uses what is actually the Asset#url. Ideally Helpers#path would use Asset#path underneath (or maybe we need a set of methods on Helpers, one for `_url`, one for `_path`).
Assets::Source always felt awkward, especially given we had to pass `source_path` arguments to its methods. In the end, I couldn’t think of any better name for this class (whose role is to provide access to the assets themselves) than just straight up `Assets`. Usage now feels much more natural.
AFAICT, we don’t expose push/promise support in Hanami 1, nor do we have plans to do so in Hanami 2, nor does any other major Ruby web framework, so we do ourselves a favour for now by removing this and greatly simplifying our helpers.
This makes it clearer what they’re returning.
If we make this a module, this is too ambiguous to be helpful
I’ve moved these to the hanami gem
Comment on lines +66 to +67
# TODO: Add tests for the File.exist? check
if config.manifest_path && File.exist?(config.manifest_path)
Copy link
Member

Choose a reason for hiding this comment

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

@jodosha FYI, this was a behavioural change I introduced as I rounded out my changes here and got things running inside Hanami, where we want to have a manifest_path configured by default.

What I need to clarify with you (when we catch up) is what the story is for local dev: whether we plan to use an auto-rebuilding or a dev server approach (the options outlined here)

If we use the auto-rebuilding approach, then I think we should probably end up with a manifest file at all times, which means we might not need this File.exist? check here. FWIW, the auto-rebuilding approach looks to be what Rails does with its jsbundling-rails gem.

However, what we might want to consider is some kind of useful error message that we show if for any reason the manifest file happens to be missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timriley FYI - In the context of hanami/assets-js#4, I had to make the following change: before starting the Esbuild watch mode, I had to create an empty manifest, otherwise the process crashes.

We can assume that there is a manifest file present.

The only exception is the race condition: if the Ruby server boots up faster than the JavaScript process to create the empty manifest.
Let's see if that happens; in that case, we can introduce a retry policy in this area.

@timriley timriley changed the title esbuild Rewrite for 2.1 and esbuild-based hanami-assets-js package Sep 6, 2023
@timriley timriley self-assigned this Sep 6, 2023
@jodosha jodosha merged commit 1c8fc3b into main Sep 7, 2023
@jodosha jodosha deleted the rewrite/esbuild branch September 7, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants