Implement BaseComponent for per-component script packs by aduth · Pull Request #5475 · 18F/identity-idp

aduth · 2021-10-07T00:57:01Z

Why: To give developers a simple way to write accompanying JavaScript for a ViewComponent, in a way which limits boilerplate and supports potential reuse for view components outside Rails. Also further reinforces the idea of components as self-contained units, limiting the amount of JavaScript rendered to a page to only that which is used in the page.

Related resources:

**Why**: To give developers a simple way to write accompanying JavaScript for a ViewComponent, in a way which limits boilerplate and supports potential reuse for view components outside Rails. Also further reinforces the idea of components as self-contained units, limiting the amount of JavaScript rendered to a page to only that which is used in the page.

aduth · 2021-10-07T00:59:36Z

app/components/base_component.rb

+      define_method 'render_in' do |view_context, &block|
+        BaseComponent.rendered_scripts |= [script]
+        super(view_context, &block)
+      end


Room for micro-optimization here: We only need to append the script once for the first call to render_in. It'd be nice if it could be possible to unwrap the method or at least short-circuit as quickly / efficiently as we can here after the first call.

aduth · 2021-10-07T12:30:38Z

app/components/base_component.rb

@@ -0,0 +1,14 @@
+class BaseComponent < ViewComponent::Base
+  @rendered_scripts = []


Class variable is a bad idea, since classes won't be reloaded on each request in production (config.cache_classes), so rendered scripts would be persist between requests. Better idea may be to call a method / variable on view_context within the wrapped render_in.

Changed to use view_context in f1c022d.

aduth · 2021-10-07T12:37:15Z

Hm, unfortunately, it looks like USWDS components which are "accordion-like" (e.g. the "Official USA website" banner) assume that the accordion scripts have been loaded and initialized, so loading them only when an accordion component is present causes some unexpected breakage.

Ideally the banner would be self-contained, so that running banner.on() would include whatever accordion behaviors it's expecting.

In the meantime, perhaps...

I leave in the scripts support, but back out the specific accordion_component.js implementation for now
- This pull request is partially intended in support of LG-5193, where I'd hoped to use the approach for componentization of the international phone field.
Or: Refactor banner (and other accordion-likes) as a component, and make sure that their scripts include the prerequisite accordion initialization.

Required for Babel to be used to transform source https://github.com/rails/webpacker/blob/e0c998e2aa7f096709eaa2a7f2d7e29d413abed3/package/rules/babel.js#L10

zachmargolis

LGTM

zachmargolis · 2021-10-07T15:36:05Z

app/components/base_component.rb

@@ -0,0 +1,10 @@
+class BaseComponent < ViewComponent::Base
+  class << self
+    def renders_script(script = self.name.underscore)


WDYT about removing the default script name? I think it would be kinder to our future selves to trace if we saw app/components/accordion_component.js in component (or even just accordion_component.js)

WDYT about removing the default script name? I think it would be kinder to our future selves to trace if we saw app/components/accordion_component.js in component (or even just accordion_component.js)

By this, do you mean having the component specify the name of the script(s) it renders?

e.g. instead of render_script, something like renders_script 'accordion_component' ?

yup! I was thinking super extra explicit, like this even

renders_script 'app/components/accordion_component.js'

or maybe renders_script './accordion_component.js'

We could make that work, but I expect it'd require some faking, since the scripts are meant to be the names of the packs generated by Webpack, which are just the base name of the script itself (source).

That being said:

Since we have to brute-force recreate this behavior in config/webpack/environment.js to add our additional component scripts, we could customize this as we want.

Thinking in terms of how we might someday reuse these components outside Rails, it'd probably be best to avoid assumptions about Webpacker.

To this latter point, this is also part of the reason I'd tried to use the class variable and avoid view_context, to avoid assuming some helper javascript_packs_tag_once exists. With what you mention here, what I'm thinking is maybe we check for an abstract method in the view context which is assumed to be implemented in a consuming project. In Rails, we'd implement it by taking a path name and mapping it to the Webpacker pack.

define_method 'render_in' do |view_context, &block| view_context.render_component_script(script) if view_context.respond_to?(:render_component_script) super(view_context, &block) end

module ScriptHelper def render_component_script(path) javascript_packs_tag_once(File.basename(path, ".*")) end end

Didn't mean to focus on the extension or path, I just mean having a filename to try to trace, I think even having renders_script 'accordion_component' would be clearer than renders_script

Hm, on the other hand, the extra-explicitness feels rather at odds with how ViewComponent already works with assuming a template file .html.erb of the same base name as the class implementation (source) 🤔

This implementation was meant to build off that:

Before:

accordion_component.rb accordion_component.html.erb

After:

accordion_component.rb accordion_component.html.erb accordion_component.js

This is not the hill I die on, I think the rails magic pattern of "just know what the filename is supposed to be" is convenient but not something I'd extend. It's common enough that it's not worth fighting against

Hm, in that case, for now I think I lie more on the side of keeping it implicit, at least for consistency with ViewComponent and what it refers to as "sidecar files".

Regardless, I did favor the idea of an abstract render_component_script method, so I implemented some simple aliasing in 138f944.

zachmargolis · 2021-10-07T15:37:15Z

spec/components/base_component_spec.rb

@@ -0,0 +1,43 @@
+require 'rails_helper'
+
+RSpec.describe BaseComponent, type: :component do


do we do anything for type: :component tests?

This pulls in ViewComponent and Capybara test helpers.

identity-idp/spec/rails_helper.rb

Lines 43 to 44 in 54fc578

config.include ViewComponent::TestHelpers, type: :component

config.include Capybara::RSpecMatchers, type: :component

Reduce tight coupling to Rails/Webpacker

Until we can find a way to incorporate implicit dependency from accordion-like UI such as banner

aduth · 2021-10-07T16:49:24Z

Hm, unfortunately, it looks like USWDS components which are "accordion-like" (e.g. the "Official USA website" banner) assume that the accordion scripts have been loaded and initialized, so loading them only when an accordion component is present causes some unexpected breakage.

For now, I reverted the accordion script in eb2d5c5. Which means this pull request doesn't yet do anything functional, but as mentioned previously, I'd already planned to (and have since started to) build on this work for LG-5192, so it'd still prove valuable as a basis.

- No separate call to render_script required - Builds on existing ViewComponent behavior - Computed once as class variable - Memoize enqueue

aduth commented Oct 7, 2021

View reviewed changes

aduth added 2 commits October 6, 2021 21:13

parentheses for lint appeasement

a1b0430

Try to improve test isolation for BaseComponent

7b99775

aduth commented Oct 7, 2021

View reviewed changes

aduth added 3 commits October 7, 2021 08:41

Add feature spec coverage for toggled USA banner

1015161

Use view context to enqueue scripts

f1c022d

Add components to additional_paths

1357eb6

Required for Babel to be used to transform source https://github.com/rails/webpacker/blob/e0c998e2aa7f096709eaa2a7f2d7e29d413abed3/package/rules/babel.js#L10

zachmargolis approved these changes Oct 7, 2021

View reviewed changes

aduth added 3 commits October 7, 2021 12:30

Opt-in component scripts to type-checking

a2518cf

Abstract component script rendering

138f944

Reduce tight coupling to Rails/Webpacker

Revert accordion script

eb2d5c5

Until we can find a way to incorporate implicit dependency from accordion-like UI such as banner

Use sidecar_files API for loading adjacent scripts

23383d2

- No separate call to render_script required - Builds on existing ViewComponent behavior - Computed once as class variable - Memoize enqueue

aduth mentioned this pull request Oct 12, 2021

LG-5193: Extract PhoneInputComponent for reuse #5481

Merged

aduth merged commit fe128d2 into main Oct 14, 2021

aduth deleted the aduth-component-scripts branch October 14, 2021 13:42

aduth mentioned this pull request Feb 4, 2022

Use ButtonComponent for new account actions #5910

Merged

		@@ -0,0 +1,14 @@
		class BaseComponent < ViewComponent::Base
		@rendered_scripts = []

		@@ -0,0 +1,43 @@
		require 'rails_helper'

		RSpec.describe BaseComponent, type: :component do

	config.include ViewComponent::TestHelpers, type: :component
	config.include Capybara::RSpecMatchers, type: :component

Conversation

aduth commented Oct 7, 2021

Uh oh!

aduth Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Oct 7, 2021

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Oct 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aduth Oct 7, 2021 •

edited

Loading

aduth Oct 7, 2021 •

edited

Loading

zachmargolis Oct 7, 2021 •

edited

Loading