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

Feature Adding AssetPipeline & ImportMaps #1358

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

crimson-knight
Copy link
Member

Description of the Change

This is a big step forward in modern JS for Amber!

This change adds the new import maps feature and the asset_pipeline shard for new apps by default. This also has some minimal updates to get the existing client_reload feature working and get the

Benefits

  • No build step during deployments, but allows JS locally and from CDNs (npm, etc.)
  • This fingerprints asset files so when files change, the links/references to the files are automatically updated to more easily break any browser caching that is configured.

Possible Drawbacks

Using module-based JS does require a different approach than what most backend devs are probably comfortable with. If you're accustomed to the standard of just putting plain JS into a file and it executes on it own, this is going to be a bit different.

…zes the basic JS to allow for autoreloading & the minimal JS for CRUD
@crimson-knight crimson-knight changed the title Feature/adding asset pipeline Feature Adding AssetPipeline & ImportMaps Nov 14, 2023
Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

lgtm. 💯 one suggestion is to use assets directory instead of src for familiarity by Rails developers.

@@ -0,0 +1,15 @@
require "asset_pipeline"

FRONT_LOADER = AssetPipeline::FrontLoader.new(js_source_path: Path["src/javascript"], js_output_path: Path["public"]) do |import_maps|
Copy link
Member

Choose a reason for hiding this comment

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

wdyt of assets/javascript instead?

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 kicked that around. If the deployment doesn't require the entire code base be cloned, then it makes total sense.
But, wouldn't most dockerized/containerized deploys mean the entire code base was cloned into the image and then deployed?

Copy link
Member

Choose a reason for hiding this comment

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

I would just copy the assets directory to the docker image instead of src/javascript. I think it will actually make creating the image easier.

@@ -28,8 +28,9 @@ html

.main== content

script src="https://cdn.jsdelivr.net/npm/bootstrap@5.0.2/dist/js/bootstrap.bundle.min.js"
== FRONT_LOADER.render_import_map_tag
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit cosmetic, but maybe a helper function like import_map_tag instead of a constant declared in the initializer could hide a bit of implementation details here.

If the plan is to have import maps as a built in feature, maybe would be worth to deal with it like Amber does with routes and settings, a file config/import_maps.cr with:

Amber::Server.import_maps(source_dir: "src/javascript", output_dir: "public") do |import_maps|
  ...
end

OR, to not fill Amber::Server with frontend stuff:

Amber::ImportMaps.configure(source_dir: "src/javascript", output_dir: "public") do
...

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.

3 participants