Add Append StyleSheet Pack Tag Helper #144

pulkitkkr · 2022-06-10T17:24:42Z

Summary

To add stylesheets for the new filesystem-based Component Registry API, We need a stylesheet to append helper. This PR Adds the stylesheet_append_tag helper to Shakapacker Codebase

tomdracz · 2022-06-12T08:42:15Z

lib/webpacker/helper.rb

@@ -158,7 +158,26 @@ def preload_pack_asset(name, **options)
  def stylesheet_pack_tag(*names, **options)
    return "" if Webpacker.inlining_css?

+    if @stylesheet_pack_tag_loaded
+      raise "To prevent duplicated stylesheet on the page, you should call stylesheet_pack_tag only once on the page. " \


I would rather not bring this raise back at this point. Duplicate stylesheets are less problematic and I don't want to bring in a breaking change for any users that relied on this raise not being here.

We'd be shipping this PR in a minor release so avoiding breaking changes is preferable.

Hey @tomdracz! I am removing the raise for now :) But Still keeping it as a warning in logs :)

tomdracz · 2022-06-12T08:44:44Z

lib/webpacker/helper.rb

+    stylesheet_link_tag(*appended_stylesheets, **options)
+  end
+
+  def append_stylesheet_pack_tag(*names)


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

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)".

@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:

String of the pack name

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?

@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

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

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

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

See #39 (comment)

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' %>

justin808 · 2022-06-13T08:50:10Z

@pulkitkkr you have CI issues.

justin808 · 2022-06-15T09:21:55Z

@pulkitkkr we need:

Tests
Documentation

tomdracz

Couple more comments added

tomdracz · 2022-06-16T08:53:46Z

lib/webpacker/helper.rb

+    end
+
+    requested_packs = sources_from_manifest_entrypoints(names, type: :stylesheet)
+    appended_packs = available_sources_from_manifest_entrypoints(@stylesheet_pack_tag_queue || [], type: :stylesheet)


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?

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

tomdracz · 2022-06-16T08:54:51Z

lib/webpacker/helper.rb

@@ -184,6 +207,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


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

@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.

justin808 · 2022-06-24T02:35:59Z

README.md

+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!
+


The fully copy/paste replace is a bit awkward.

All the magic of splitting up the code was automatic!

Maybe:
All the magic of splitting up the CSS was automatic!

However, I think the docs can be more concise by discussing both helpers together.

Plus the docs for the append_stylesheet_pack_tag and stylesheet_pack_tag should mention the media type issue.

justin808 · 2022-06-24T02:40:24Z

lib/webpacker/helper.rb

+    stylesheet_link_tag(*appended_stylesheets, **options)
+  end
+
+  def append_stylesheet_pack_tag(*names)


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

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

See #39 (comment)

justin808 · 2022-06-24T02:45:28Z

test/helper_test.rb

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


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.

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

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

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

@alexeyr We already have a one for different names test_stylesheet_pack_tag_multiple_invocations_are_allowed

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

Updated The Test to handle this scenario :)

…fferent media attributes

alexeyr · 2022-06-30T15:22:22Z

README.md

 ```

+


Trivial, but this extra line was probably added accidentally. If you do change tests, may as well remove it.

Removed it! My bad.

alexeyr · 2022-06-30T15:38:31Z

README.md

 ```

 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!


Actually, there is something left over from @justin808's comments here: "splitting up code and CSS"

or "code and stylesheets"

Updated it 😄

alexeyr · 2022-06-30T15:45:33Z

lib/webpacker/helper.rb

@@ -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. " \


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?

As discussed over DM, removed the warning for now 😄

This PR just adds Changelog Text for append_stylesheet_pack_tag added in #144

justin808 · 2022-06-30T20:23:28Z

README.md


 **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.


justin808 · 2022-06-30T20:25:16Z

lib/webpacker/helper.rb

+  def append_stylesheet_pack_tag(*names)
+    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"


Please refer to the documentation for stylesheet_pack_tag for more information.

Add Append StyleSheet Pack Tag

6873d42

tomdracz requested changes Jun 12, 2022

View reviewed changes

Address Comments

8c1d958

tomdracz reviewed Jun 16, 2022

View reviewed changes

pulkitkkr added 3 commits June 17, 2022 10:46

Address Comments

eed566f

Add Test Case for append_stylesheet_pack_tag

4b8c45d

Add Documentation

1f72772

pulkitkkr requested review from justin808 and tomdracz June 17, 2022 14:58

justin808 requested changes Jun 24, 2022

View reviewed changes

pulkitkkr added 2 commits June 29, 2022 18:50

Add Test for stylesheet_pack_tag can be called multiple times with di…

32e4d3f

…fferent media attributes

Update Documentation

1d2157e

pulkitkkr force-pushed the pulkitkkr/add-stylesheet-append-tag branch from 4e6a46b to 1d2157e Compare June 29, 2022 23:10

pulkitkkr requested a review from justin808 June 29, 2022 23:12

alexeyr reviewed Jun 30, 2022

View reviewed changes

pulkitkkr force-pushed the pulkitkkr/add-stylesheet-append-tag branch 2 times, most recently from 6b7ea3e to 1e1e099 Compare June 30, 2022 15:33

pulkitkkr requested a review from alexeyr June 30, 2022 15:34

alexeyr approved these changes Jun 30, 2022

View reviewed changes

alexeyr reviewed Jun 30, 2022

View reviewed changes

Address Comments

dd6d56a

pulkitkkr force-pushed the pulkitkkr/add-stylesheet-append-tag branch from 1e1e099 to dd6d56a Compare June 30, 2022 15:40

alexeyr reviewed Jun 30, 2022

View reviewed changes

pulkitkkr added 2 commits June 30, 2022 12:08

Remove Warning related to Multiple Invocation of stylesheet_pack_tag

118686b

Remove assert_not_raised

7e8acbd

alexeyr merged commit 7bf7b0f into master Jun 30, 2022

alexeyr deleted the pulkitkkr/add-stylesheet-append-tag branch June 30, 2022 17:04

pulkitkkr mentioned this pull request Jun 30, 2022

Add Changelog for append_stylesheet_pack_tag helper #152

Merged

alexeyr-ci1 pushed a commit that referenced this pull request Jun 30, 2022

Add Changelog for append_stylesheet_pack_tag helper (#152)

184ab84

This PR just adds Changelog Text for append_stylesheet_pack_tag added in #144

justin808 reviewed Jun 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Append StyleSheet Pack Tag Helper #144

Add Append StyleSheet Pack Tag Helper #144

pulkitkkr commented Jun 10, 2022

tomdracz Jun 12, 2022

pulkitkkr Jun 14, 2022

tomdracz Jun 12, 2022

pulkitkkr Jun 14, 2022

justin808 Jun 15, 2022

tomdracz Jun 16, 2022

pulkitkkr Jun 17, 2022

justin808 Jun 24, 2022

pulkitkkr Jun 29, 2022

justin808 commented Jun 13, 2022

justin808 commented Jun 15, 2022

tomdracz left a comment

tomdracz Jun 16, 2022

pulkitkkr Jun 17, 2022 •

edited

Loading

tomdracz Jun 16, 2022

pulkitkkr Jun 17, 2022

justin808 Jun 24, 2022

justin808 Jun 24, 2022

justin808 Jun 24, 2022

pulkitkkr Jun 29, 2022

alexeyr Jun 30, 2022

alexeyr Jun 30, 2022

pulkitkkr Jun 30, 2022

alexeyr Jun 30, 2022

pulkitkkr Jun 30, 2022

alexeyr Jun 30, 2022

pulkitkkr Jun 30, 2022

alexeyr Jun 30, 2022

alexeyr Jun 30, 2022

pulkitkkr Jun 30, 2022

alexeyr Jun 30, 2022

pulkitkkr Jun 30, 2022

justin808 Jun 30, 2022

justin808 Jun 30, 2022

		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!


		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.

Add Append StyleSheet Pack Tag Helper #144

Add Append StyleSheet Pack Tag Helper #144

Conversation

pulkitkkr commented Jun 10, 2022

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 commented Jun 13, 2022

justin808 commented Jun 15, 2022

tomdracz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulkitkkr Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulkitkkr Jun 17, 2022 •

edited

Loading