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

Add Append StyleSheet Pack Tag Helper #144

Merged
merged 10 commits into from
Jun 30, 2022
12 changes: 9 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -249,36 +249,42 @@ While this also generally applies to `stylesheet_pack_tag`, you may use multiple
<%= stylesheet_pack_tag 'print', media: 'print' %>
```

#### View Helper `append_javascript_pack_tag`
#### View Helper `append_javascript_pack_tag` and `append_stylesheet_pack_tag`

If you need configure your pack names from the view for a route or partials, then you will need some logic to ensure you call the helpers only once with multiple arguments. The new view helper `append_javascript_pack_tag` can solve this problem. This helper will queue up packs when the `javascript_pack_tag` is finally used.
If you need configure your script pack names or stylesheet pack names from the view for a route or partials, then you will need some logic to ensure you call the helpers only once with multiple arguments. The new view helpers, `append_javascript_pack_tag` and `append_stylesheet_pack_tag` can solve this problem. The helper `append_javascript_pack_tag` will queue up script packs when the `javascript_pack_tag` is finally used. Similarly,`append_stylesheet_pack_tag` will queue up style packs when the `stylesheet_pack_tag` is finally used.

Main view:
```erb
<% append_javascript_pack_tag 'calendar' %>
<% append_stylesheet_pack_tag 'calendar' %>
```

Some partial:
```erb
<% append_javascript_pack_tag 'map' %>
<% append_stylesheet_pack_tag 'map' %>
```

And the main layout has:
```erb
<%= javascript_pack_tag 'application' %>
<%= stylesheet_pack_tag 'application' %>
```

is the same as using this in the main layout:

```erb
<%= javascript_pack_tag 'calendar', 'map', application' %>
<%= stylesheet_pack_tag 'calendar', 'map', application' %>
```

However, you typically can't do that in the main layout, as the view and partial codes will depend on the route.

Thus, you can distribute the logic of what packs are needed for any route. All the magic of splitting up the code was automatic!
Thus, you can distribute the logic of what packs are needed for any route. All the magic of splitting up the code and CSS was automatic!

**Important:** `append_javascript_pack_tag` can be used anywhere in your application as long as it is executed BEFORE the `javascript_pack_tag`. If you attempt to call `append_javascript_pack_tag` helper after `javascript_pack_tag`, an error will be raised. You should have only a single `javascript_pack_tag` invocation in your page load.

**Important** Similar to `append_javascript_pack_tag`, the `append_stylesheet_pack_tag` should be used anywhere in your application as long as it is executed BEFORE the `stylesheet_pack_tag`. However, if you attempt to call `append_stylesheet_pack_tag` helper after `stylesheet_pack_tag`, an error will be **NOT** be raised.
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect.


The typical issue is that your layout might reference some partials that need to configure packs. A good way to solve this problem is to use `content_for` to ensure that the code to render your partial comes before the call to `javascript_pack_tag`.

Expand Down
26 changes: 25 additions & 1 deletion lib/webpacker/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,27 @@ def preload_pack_asset(name, **options)
def stylesheet_pack_tag(*names, **options)
return "" if Webpacker.inlining_css?

stylesheet_link_tag(*sources_from_manifest_entrypoints(names, type: :stylesheet), **options)
if @stylesheet_pack_tag_loaded
puts "To prevent duplicated stylesheet on the page, you should call stylesheet_pack_tag only once on the page. " \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, won't you get the warning here even for the perfectly fine case of

While this also generally applies to stylesheet_pack_tag, you may use multiple calls to stylesheet_pack_tag if, say, you require multiple <style> tags for different output media:

Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed over DM, removed the warning for now 😄

"Please refer to https://github.com/shakacode/shakapacker/blob/master/README.md#usage for the usage guide"
end

requested_packs = sources_from_manifest_entrypoints(names, type: :stylesheet)
appended_packs = available_sources_from_manifest_entrypoints(@stylesheet_pack_tag_queue || [], type: :stylesheet)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need any deduplication? Can names passed and what's in @stylesheet_pack_tag_queue reference the same files that we would load twice?

Copy link
Contributor Author

@pulkitkkr pulkitkkr Jun 17, 2022

Choose a reason for hiding this comment

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

That's a good catch, Handled it by using stylesheet_link_tag(*(requested_packs | appended_packs), **options) now.


@stylesheet_pack_tag_loaded = true

stylesheet_link_tag(*(requested_packs | appended_packs), **options)
end

def append_stylesheet_pack_tag(*names)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think we need more than just names here. If you look at original issue around multiple pack tags, one of the requests was supporting different "media" attributes.
Wondering if we should queue up files based on media attribute in the same way we queue and organise js packs on defer attr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can Queue Up the files based on media attributes as a separate Feature Pull Request as, unlike deferred, it's not a boolean value and can have and, not or a comma.

Example: media="screen and (max-aspect-ratio:16/9)".

Copy link
Member

Choose a reason for hiding this comment

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

@tomdracz I'm not convinced how many people would need both the flexibility of the append_stylesheet_pack_tag with different media attributes.

Let's wait for feedback.

If necessary, we release an update where the *names arg can be either:

  1. String of the pack name
  2. Hash, where the keys are the media and the values are arrays of packs.

@tomdracz do you agree that this would be a backwards compatible update if needed?

@pulkitkkr are we ready to merge and release this one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@justin808 If I remember correctly we've removed the raise in the first place to support different media attrs. Agree that if we've got that in place, then having it in append_ helper is less crucial.

The update if needed should be backwards-compatible, yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomdracz should we document the ordering for appended_packs and requested_packs ?

Copy link
Member

Choose a reason for hiding this comment

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

@pulkitkkr we need to document what happens when this happens:

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

See #39 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have :

While this also generally applies to `stylesheet_pack_tag`, you may use multiple calls to stylesheet_pack_tag if, say, you require multiple <style> tags for different output media:

``` erb
<%= stylesheet_pack_tag 'application', media: 'screen' %>
<%= stylesheet_pack_tag 'print', media: 'print' %>

if @stylesheet_pack_tag_loaded
raise "You can only call append_stylesheet_pack_tag before stylesheet_pack_tag helper. " \
"Please refer to https://github.com/shakacode/shakapacker/blob/master/README.md#usage for the usage guide"
Copy link
Member

Choose a reason for hiding this comment

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

Please refer to the documentation for stylesheet_pack_tag for more information.

end

@stylesheet_pack_tag_queue ||= []
@stylesheet_pack_tag_queue.concat names
end

def append_javascript_pack_tag(*names, defer: true)
Expand All @@ -184,6 +204,10 @@ def sources_from_manifest_entrypoints(names, type:)
names.map { |name| current_webpacker_instance.manifest.lookup_pack_with_chunks!(name.to_s, type: type) }.flatten.uniq
end

def available_sources_from_manifest_entrypoints(names, type:)
names.map { |name| current_webpacker_instance.manifest.lookup_pack_with_chunks(name.to_s, type: type) }.flatten.compact.uniq
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pulkitkkr What's the rationale for adding this? Why would we use non-raising method that does not indicate pack missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomdracz We are introducing a new feature to react_on_rails that automatically loads the packs associated with the requested component.

Eg: Developer used react_component("MyComponent", props )

This will automatically load, generated/MyComponent pack using append_javascript_pack and append_stylesheet_pack

While testing this on one of the projects, we discovered there were few component packs with js only, i.e. No stylesheets. We want to use a non-error-raising lookup to handle such a scenario.

end

def resolve_path_to_image(name, **options)
path = name.starts_with?("static/") ? name : "static/#{name}"
path_to_asset(current_webpacker_instance.manifest.lookup!(path), options)
Expand Down
45 changes: 45 additions & 0 deletions test/helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,5 +204,50 @@ def test_stylesheet_pack_tag_multiple_invocations_are_allowed
assert_equal \
hello_stimulus_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"),
stimulus_style

assert_nothing_raised do
stylesheet_pack_tag(:application)
stylesheet_pack_tag(:hello_stimulus)
end
end

def test_stylesheet_pack_with_append
append_stylesheet_pack_tag(:hello_stimulus)

assert_equal \
(application_stylesheet_chunks + hello_stimulus_stylesheet_chunks).uniq.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"),
stylesheet_pack_tag(:application)
end

def test_stylesheet_pack_with_duplicate_append
append_stylesheet_pack_tag(:hello_stimulus)
append_stylesheet_pack_tag(:application)

assert_equal \
(application_stylesheet_chunks + hello_stimulus_stylesheet_chunks).uniq.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"),
stylesheet_pack_tag(:application)
end
Copy link
Member

Choose a reason for hiding this comment

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

Tests don't address:

  • stylesheet_pack_tag can be called multiple times with different media attributes, while javascript_pack_tag can be called only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test test_multiple_stylesheet_pack_with_different_media_attr and for javascript_pack_tag we already have test test_javascript_pack_tag_multiple_invocations

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is assert_nothing_raised enough there? Can we also test what happens?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(and possibly both with the same and different stylesheet names).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexeyr We already have a one for different names test_stylesheet_pack_tag_multiple_invocations_are_allowed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant with different names and different media attributes, which that test doesn't do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated The Test to handle this scenario :)


def test_multiple_stylesheet_pack_with_different_media_attr
app_style = stylesheet_pack_tag(:application)
app_style_with_media = stylesheet_pack_tag(:application, media: "print")
hello_stimulus_style_with_media = stylesheet_pack_tag(:hello_stimulus, media: "all")

assert_equal \
application_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"),
app_style

assert_equal \
application_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk, media: "print") }.join("\n"),
app_style_with_media

assert_equal \
hello_stimulus_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk, media: "all") }.join("\n"),
hello_stimulus_style_with_media

assert_nothing_raised do
stylesheet_pack_tag(:application)
stylesheet_pack_tag(:application, media: "print")
end
end
end