From aecf8767136d86b6367825c4060d2db20227f41d Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 6 Oct 2021 20:55:46 -0400 Subject: [PATCH 01/10] Implement BaseComponent for per-component script packs **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. --- app/components/accordion_component.js | 3 ++ app/components/accordion_component.rb | 3 +- app/components/base_component.rb | 14 +++++++ app/javascript/app/components/index.js | 4 +- app/javascript/packages/es5-safe/package.json | 2 +- app/views/layouts/base.html.erb | 5 ++- config/webpack/environment.js | 6 +++ package.json | 1 + spec/components/base_component_spec.rb | 41 +++++++++++++++++++ yarn.lock | 2 +- 10 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 app/components/accordion_component.js create mode 100644 app/components/base_component.rb create mode 100644 spec/components/base_component_spec.rb diff --git a/app/components/accordion_component.js b/app/components/accordion_component.js new file mode 100644 index 00000000000..5a1de061db6 --- /dev/null +++ b/app/components/accordion_component.js @@ -0,0 +1,3 @@ +import { accordion } from 'identity-style-guide'; + +accordion.on(); diff --git a/app/components/accordion_component.rb b/app/components/accordion_component.rb index 3bbb35a0377..4225dbf6d3f 100644 --- a/app/components/accordion_component.rb +++ b/app/components/accordion_component.rb @@ -1,4 +1,5 @@ -class AccordionComponent < ViewComponent::Base +class AccordionComponent < BaseComponent + renders_script renders_one :header def initialize diff --git a/app/components/base_component.rb b/app/components/base_component.rb new file mode 100644 index 00000000000..fe3f989bc3d --- /dev/null +++ b/app/components/base_component.rb @@ -0,0 +1,14 @@ +class BaseComponent < ViewComponent::Base + @rendered_scripts = [] + + class << self + attr_accessor :rendered_scripts + + def renders_script(script = self.name.underscore) + define_method 'render_in' do |view_context, &block| + BaseComponent.rendered_scripts |= [script] + super(view_context, &block) + end + end + end +end diff --git a/app/javascript/app/components/index.js b/app/javascript/app/components/index.js index 45c70f9dba0..8aabc4ba14c 100644 --- a/app/javascript/app/components/index.js +++ b/app/javascript/app/components/index.js @@ -1,9 +1,9 @@ -import { accordion, banner, navigation, skipnav } from 'identity-style-guide'; +import { banner, navigation, skipnav } from 'identity-style-guide'; import domready from 'domready'; import Modal from './modal'; window.LoginGov = window.LoginGov || {}; window.LoginGov.Modal = Modal; -const components = [accordion, banner, navigation, skipnav]; +const components = [banner, navigation, skipnav]; domready(() => components.forEach((component) => component.on())); diff --git a/app/javascript/packages/es5-safe/package.json b/app/javascript/packages/es5-safe/package.json index 0ca0483e308..babe449f67d 100644 --- a/app/javascript/packages/es5-safe/package.json +++ b/app/javascript/packages/es5-safe/package.json @@ -8,7 +8,7 @@ }, "dependencies": { "acorn": "^6.4.2", - "fast-glob": "^3.2.4", + "fast-glob": "^3.2.7", "p-all": "^3.0.0" } } diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb index d7b95d4ef8e..39144c3b174 100644 --- a/app/views/layouts/base.html.erb +++ b/app/views/layouts/base.html.erb @@ -19,12 +19,12 @@ <%= yield(:meta_tags) if content_for?(:meta_tags) %> - + <%= content_tag( 'title', content_for?(:title) ? raw("#{yield(:title)} - #{APP_NAME}") : APP_NAME, ) %> - + <%= nonced_javascript_tag do %> document.documentElement.className = document.documentElement.className.replace(/\bno-js\b/, 'js'); @@ -103,6 +103,7 @@ <%= content_tag 'script', '', data: { analytics_endpoint: api_logger_path } %> <%= javascript_packs_tag_once 'application', prepend: true %> + <%= javascript_packs_tag_once *BaseComponent.rendered_scripts %> <%= render_javascript_pack_once_tags %> <%= render 'shared/dap_analytics' if IdentityConfig.store.participate_in_dap && !session_with_trust? %> diff --git a/config/webpack/environment.js b/config/webpack/environment.js index 028ab782e56..cfc4d214ab4 100644 --- a/config/webpack/environment.js +++ b/config/webpack/environment.js @@ -1,6 +1,12 @@ +const { parse, resolve } = require('path'); const { environment } = require('@rails/webpacker'); +const { sync: glob } = require('fast-glob'); const RailsI18nWebpackPlugin = require('@18f/identity-rails-i18n-webpack-plugin'); +glob('app/components/*.js').forEach((path) => { + environment.entry[parse(path).name] = resolve(path); +}); + environment.loaders.delete('file'); environment.loaders.delete('nodeModules'); environment.loaders.delete('moduleSass'); diff --git a/package.json b/package.json index aad8c2d3b60..6a792beb2ac 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "cleave.js": "^1.6.0", "clipboard": "^2.0.6", "domready": "^1.0.8", + "fast-glob": "^3.2.7", "focus-trap": "^6.2.3", "identity-style-guide": "^6.2.0", "intl-tel-input": "^17.0.8", diff --git a/spec/components/base_component_spec.rb b/spec/components/base_component_spec.rb new file mode 100644 index 00000000000..935da524ad6 --- /dev/null +++ b/spec/components/base_component_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe BaseComponent, type: :component do + class ExampleComponent < described_class + def render_in(...) + '' + end + end + + after { described_class.rendered_scripts = [] } + + it 'initializes rendered scripts as empty array' do + expect(described_class.rendered_scripts).to eq([]) + end + + it 'does nothing when rendered' do + render_inline(ExampleComponent.new) + + expect(described_class.rendered_scripts).to eq([]) + end + + context 'declares rendered script' do + class ExampleComponentWithScript < ExampleComponent; renders_script; end + + it 'adds script to class variable when rendered' do + render_inline(ExampleComponentWithScript.new) + + expect(described_class.rendered_scripts).to eq(['example_component_with_script']) + end + end + + context 'declares named rendered script' do + class ExampleComponentWithNamedScript < ExampleComponent; renders_script 'my_script'; end + + it 'adds script to class variable when rendered' do + render_inline(ExampleComponentWithNamedScript.new) + + expect(described_class.rendered_scripts).to eq(['my_script']) + end + end +end diff --git a/yarn.lock b/yarn.lock index 78054bcce12..e54dfc2f642 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4078,7 +4078,7 @@ fast-diff@^1.1.2: resolved "https://registry.yarnpkg.com/fast-diff/-/fast-diff-1.2.0.tgz#73ee11982d86caaf7959828d519cfe927fac5f03" integrity sha512-xJuoT5+L99XlZ8twedaRf6Ax2TgQVxvgZOYoPKqZufmJib0tL2tegPBOZb1pVNgIhlqDlA0eO0c3wBvQcmzx4w== -fast-glob@^3.1.1, fast-glob@^3.2.4: +fast-glob@^3.1.1, fast-glob@^3.2.7: version "3.2.7" resolved "https://registry.yarnpkg.com/fast-glob/-/fast-glob-3.2.7.tgz#fd6cb7a2d7e9aa7a7846111e85a196d6b2f766a1" integrity sha512-rYGMRwip6lUMvYD3BTScMwT1HtAs2d71SMv66Vrxs0IekGZEjhM0pcMfjQPnknBt2zeCwQMEupiN02ZP4DiT1Q== From a1b043063388e711ccaecdcf47515cd276a3fe2d Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 6 Oct 2021 21:13:24 -0400 Subject: [PATCH 02/10] parentheses for lint appeasement --- app/views/layouts/base.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb index 39144c3b174..1bc63696a91 100644 --- a/app/views/layouts/base.html.erb +++ b/app/views/layouts/base.html.erb @@ -102,8 +102,8 @@ <% end %> <%= content_tag 'script', '', data: { analytics_endpoint: api_logger_path } %> - <%= javascript_packs_tag_once 'application', prepend: true %> - <%= javascript_packs_tag_once *BaseComponent.rendered_scripts %> + <%= javascript_packs_tag_once('application', prepend: true) %> + <%= javascript_packs_tag_once(*BaseComponent.rendered_scripts) %> <%= render_javascript_pack_once_tags %> <%= render 'shared/dap_analytics' if IdentityConfig.store.participate_in_dap && !session_with_trust? %> From 7b997755fbd22c71cff9bf6b5ba93ebec28e4240 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 6 Oct 2021 21:21:43 -0400 Subject: [PATCH 03/10] Try to improve test isolation for BaseComponent --- spec/components/base_component_spec.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/spec/components/base_component_spec.rb b/spec/components/base_component_spec.rb index 935da524ad6..97e42aebb17 100644 --- a/spec/components/base_component_spec.rb +++ b/spec/components/base_component_spec.rb @@ -7,16 +7,17 @@ def render_in(...) end end - after { described_class.rendered_scripts = [] } + let!(:initial_rendered_scripts) { described_class.rendered_scripts } + after { described_class.rendered_scripts = initial_rendered_scripts } - it 'initializes rendered scripts as empty array' do - expect(described_class.rendered_scripts).to eq([]) + def rendered_scripts + described_class.rendered_scripts - initial_rendered_scripts end it 'does nothing when rendered' do render_inline(ExampleComponent.new) - expect(described_class.rendered_scripts).to eq([]) + expect(rendered_scripts).to eq([]) end context 'declares rendered script' do @@ -25,7 +26,7 @@ class ExampleComponentWithScript < ExampleComponent; renders_script; end it 'adds script to class variable when rendered' do render_inline(ExampleComponentWithScript.new) - expect(described_class.rendered_scripts).to eq(['example_component_with_script']) + expect(rendered_scripts).to eq(['example_component_with_script']) end end @@ -35,7 +36,7 @@ class ExampleComponentWithNamedScript < ExampleComponent; renders_script 'my_scr it 'adds script to class variable when rendered' do render_inline(ExampleComponentWithNamedScript.new) - expect(described_class.rendered_scripts).to eq(['my_script']) + expect(rendered_scripts).to eq(['my_script']) end end end From 1015161c85f755a9c7eadf1b07c65f7cd4dc16e0 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 08:41:25 -0400 Subject: [PATCH 04/10] Add feature spec coverage for toggled USA banner --- spec/features/visitors/js_disabled_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/features/visitors/js_disabled_spec.rb b/spec/features/visitors/js_disabled_spec.rb index 4a9a3b5066c..d4d6d90dffe 100644 --- a/spec/features/visitors/js_disabled_spec.rb +++ b/spec/features/visitors/js_disabled_spec.rb @@ -15,6 +15,10 @@ visit root_path expect(page).to have_css('#gov-banner', visible: :hidden) + + click_on t('shared.banner.how') + + expect(page).to have_css('#gov-banner', visible: true) end end end From f1c022dcda9ae870a58b3a781fce8f1ab8ad89d9 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 09:25:41 -0400 Subject: [PATCH 05/10] Use view context to enqueue scripts --- app/components/base_component.rb | 6 +----- app/views/layouts/base.html.erb | 1 - spec/components/base_component_spec.rb | 21 +++++++++++---------- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/app/components/base_component.rb b/app/components/base_component.rb index fe3f989bc3d..d0c60b78406 100644 --- a/app/components/base_component.rb +++ b/app/components/base_component.rb @@ -1,12 +1,8 @@ class BaseComponent < ViewComponent::Base - @rendered_scripts = [] - class << self - attr_accessor :rendered_scripts - def renders_script(script = self.name.underscore) define_method 'render_in' do |view_context, &block| - BaseComponent.rendered_scripts |= [script] + view_context.javascript_packs_tag_once(script) super(view_context, &block) end end diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb index 1bc63696a91..810576dc5f3 100644 --- a/app/views/layouts/base.html.erb +++ b/app/views/layouts/base.html.erb @@ -103,7 +103,6 @@ <%= content_tag 'script', '', data: { analytics_endpoint: api_logger_path } %> <%= javascript_packs_tag_once('application', prepend: true) %> - <%= javascript_packs_tag_once(*BaseComponent.rendered_scripts) %> <%= render_javascript_pack_once_tags %> <%= render 'shared/dap_analytics' if IdentityConfig.store.participate_in_dap && !session_with_trust? %> diff --git a/spec/components/base_component_spec.rb b/spec/components/base_component_spec.rb index 97e42aebb17..69622058046 100644 --- a/spec/components/base_component_spec.rb +++ b/spec/components/base_component_spec.rb @@ -7,26 +7,27 @@ def render_in(...) end end - let!(:initial_rendered_scripts) { described_class.rendered_scripts } - after { described_class.rendered_scripts = initial_rendered_scripts } + let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } + let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } - def rendered_scripts - described_class.rendered_scripts - initial_rendered_scripts + before do + allow_any_instance_of(ApplicationController).to receive(:view_context).and_return(view_context) end it 'does nothing when rendered' do - render_inline(ExampleComponent.new) + expect(view_context).not_to receive(:javascript_packs_tag_once) - expect(rendered_scripts).to eq([]) + render_inline(ExampleComponent.new) end context 'declares rendered script' do class ExampleComponentWithScript < ExampleComponent; renders_script; end it 'adds script to class variable when rendered' do - render_inline(ExampleComponentWithScript.new) + expect(view_context).to receive(:javascript_packs_tag_once). + with('example_component_with_script') - expect(rendered_scripts).to eq(['example_component_with_script']) + render_inline(ExampleComponentWithScript.new) end end @@ -34,9 +35,9 @@ class ExampleComponentWithScript < ExampleComponent; renders_script; end class ExampleComponentWithNamedScript < ExampleComponent; renders_script 'my_script'; end it 'adds script to class variable when rendered' do - render_inline(ExampleComponentWithNamedScript.new) + expect(view_context).to receive(:javascript_packs_tag_once).with('my_script') - expect(rendered_scripts).to eq(['my_script']) + render_inline(ExampleComponentWithNamedScript.new) end end end From 1357eb6c35d4c9d89eb2b80346d18d60ba0cc557 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 10:09:06 -0400 Subject: [PATCH 06/10] Add components to additional_paths Required for Babel to be used to transform source https://github.com/rails/webpacker/blob/e0c998e2aa7f096709eaa2a7f2d7e29d413abed3/package/rules/babel.js#L10 --- config/webpacker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/webpacker.yml b/config/webpacker.yml index 00f2406bf15..1814be3e611 100644 --- a/config/webpacker.yml +++ b/config/webpacker.yml @@ -8,7 +8,7 @@ default: &default # Additional paths webpack should lookup modules # ['app/assets', 'engine/foo/app/assets'] - resolved_paths: [] + additional_paths: ['app/components'] # Reload manifest.json on all requests so we reload latest compiled packs cache_manifest: false From a2518cf2c0de3466452a618ebcd67dd11255f610 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 12:30:15 -0400 Subject: [PATCH 07/10] Opt-in component scripts to type-checking --- tsconfig.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tsconfig.json b/tsconfig.json index 31ea0f46682..44f66fbe5d4 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -12,6 +12,7 @@ "target": "ESNext" }, "include": [ + "app/components", "app/javascript/app/phone-internationalization.js", "app/javascript/packages", "app/javascript/packs/doc-capture-polling.js", From 138f944cb1e0352bba6a9775360b364b49e8e95d Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 12:42:39 -0400 Subject: [PATCH 08/10] Abstract component script rendering Reduce tight coupling to Rails/Webpacker --- app/components/base_component.rb | 5 ++++- app/helpers/script_helper.rb | 2 ++ spec/components/base_component_spec.rb | 6 +++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/components/base_component.rb b/app/components/base_component.rb index d0c60b78406..6241da1c166 100644 --- a/app/components/base_component.rb +++ b/app/components/base_component.rb @@ -2,7 +2,10 @@ class BaseComponent < ViewComponent::Base class << self def renders_script(script = self.name.underscore) define_method 'render_in' do |view_context, &block| - view_context.javascript_packs_tag_once(script) + if view_context.respond_to?(:render_component_script) + view_context.render_component_script(script) + end + super(view_context, &block) end end diff --git a/app/helpers/script_helper.rb b/app/helpers/script_helper.rb index 36103ac303a..695646017cb 100644 --- a/app/helpers/script_helper.rb +++ b/app/helpers/script_helper.rb @@ -20,6 +20,8 @@ def javascript_packs_tag_once(*names, prepend: false) nil end + alias_method :render_component_script, :javascript_packs_tag_once + def render_javascript_pack_once_tags return if !@scripts diff --git a/spec/components/base_component_spec.rb b/spec/components/base_component_spec.rb index 69622058046..19e40a96114 100644 --- a/spec/components/base_component_spec.rb +++ b/spec/components/base_component_spec.rb @@ -15,7 +15,7 @@ def render_in(...) end it 'does nothing when rendered' do - expect(view_context).not_to receive(:javascript_packs_tag_once) + expect(view_context).not_to receive(:render_component_script) render_inline(ExampleComponent.new) end @@ -24,7 +24,7 @@ def render_in(...) class ExampleComponentWithScript < ExampleComponent; renders_script; end it 'adds script to class variable when rendered' do - expect(view_context).to receive(:javascript_packs_tag_once). + expect(view_context).to receive(:render_component_script). with('example_component_with_script') render_inline(ExampleComponentWithScript.new) @@ -35,7 +35,7 @@ class ExampleComponentWithScript < ExampleComponent; renders_script; end class ExampleComponentWithNamedScript < ExampleComponent; renders_script 'my_script'; end it 'adds script to class variable when rendered' do - expect(view_context).to receive(:javascript_packs_tag_once).with('my_script') + expect(view_context).to receive(:render_component_script).with('my_script') render_inline(ExampleComponentWithNamedScript.new) end From eb2d5c5b8a400737783148edff144bcefc4a8cfc Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 12:46:49 -0400 Subject: [PATCH 09/10] Revert accordion script Until we can find a way to incorporate implicit dependency from accordion-like UI such as banner --- app/components/accordion_component.js | 3 --- app/components/accordion_component.rb | 1 - app/javascript/app/components/index.js | 4 ++-- 3 files changed, 2 insertions(+), 6 deletions(-) delete mode 100644 app/components/accordion_component.js diff --git a/app/components/accordion_component.js b/app/components/accordion_component.js deleted file mode 100644 index 5a1de061db6..00000000000 --- a/app/components/accordion_component.js +++ /dev/null @@ -1,3 +0,0 @@ -import { accordion } from 'identity-style-guide'; - -accordion.on(); diff --git a/app/components/accordion_component.rb b/app/components/accordion_component.rb index 4225dbf6d3f..fdc84507e6d 100644 --- a/app/components/accordion_component.rb +++ b/app/components/accordion_component.rb @@ -1,5 +1,4 @@ class AccordionComponent < BaseComponent - renders_script renders_one :header def initialize diff --git a/app/javascript/app/components/index.js b/app/javascript/app/components/index.js index 8aabc4ba14c..45c70f9dba0 100644 --- a/app/javascript/app/components/index.js +++ b/app/javascript/app/components/index.js @@ -1,9 +1,9 @@ -import { banner, navigation, skipnav } from 'identity-style-guide'; +import { accordion, banner, navigation, skipnav } from 'identity-style-guide'; import domready from 'domready'; import Modal from './modal'; window.LoginGov = window.LoginGov || {}; window.LoginGov.Modal = Modal; -const components = [banner, navigation, skipnav]; +const components = [accordion, banner, navigation, skipnav]; domready(() => components.forEach((component) => component.on())); From 23383d279da2ded2f9ababce2f877339aabf3c46 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 7 Oct 2021 14:44:32 -0400 Subject: [PATCH 10/10] Use sidecar_files API for loading adjacent scripts - No separate call to render_script required - Builds on existing ViewComponent behavior - Computed once as class variable - Memoize enqueue --- app/components/base_component.rb | 21 ++++++++++++-------- spec/components/base_component_spec.rb | 27 +++++++++++++------------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/app/components/base_component.rb b/app/components/base_component.rb index 6241da1c166..e952ee55eae 100644 --- a/app/components/base_component.rb +++ b/app/components/base_component.rb @@ -1,13 +1,18 @@ class BaseComponent < ViewComponent::Base - class << self - def renders_script(script = self.name.underscore) - define_method 'render_in' do |view_context, &block| - if view_context.respond_to?(:render_component_script) - view_context.render_component_script(script) - end + def render_in(view_context, &block) + render_scripts_in(view_context) + super(view_context, &block) + end - super(view_context, &block) - end + def render_scripts_in(view_context) + return if @rendered_scripts + @rendered_scripts = true + if view_context.respond_to?(:render_component_script) && self.class.scripts.present? + view_context.render_component_script(*self.class.scripts) end end + + def self.scripts + @scripts ||= _sidecar_files(['js']).map { |file| File.basename(file, '.js') } + end end diff --git a/spec/components/base_component_spec.rb b/spec/components/base_component_spec.rb index 19e40a96114..c44c5a1a5b1 100644 --- a/spec/components/base_component_spec.rb +++ b/spec/components/base_component_spec.rb @@ -1,8 +1,8 @@ require 'rails_helper' RSpec.describe BaseComponent, type: :component do - class ExampleComponent < described_class - def render_in(...) + class ExampleComponent < BaseComponent + def call '' end end @@ -20,8 +20,17 @@ def render_in(...) render_inline(ExampleComponent.new) end - context 'declares rendered script' do - class ExampleComponentWithScript < ExampleComponent; renders_script; end + context 'with sidecar script' do + class ExampleComponentWithScript < BaseComponent + def call + '' + end + + def self._sidecar_files(extensions) + return ['/path/to/app/components/example_component_with_script.js'] if extensions == ['js'] + super(extensions) + end + end it 'adds script to class variable when rendered' do expect(view_context).to receive(:render_component_script). @@ -30,14 +39,4 @@ class ExampleComponentWithScript < ExampleComponent; renders_script; end render_inline(ExampleComponentWithScript.new) end end - - context 'declares named rendered script' do - class ExampleComponentWithNamedScript < ExampleComponent; renders_script 'my_script'; end - - it 'adds script to class variable when rendered' do - expect(view_context).to receive(:render_component_script).with('my_script') - - render_inline(ExampleComponentWithNamedScript.new) - end - end end