From 4c58d70b0907f0d4e09d8642f56d8457e0316fad Mon Sep 17 00:00:00 2001 From: Randall Reed Date: Fri, 22 Jun 2018 15:55:45 -0400 Subject: [PATCH 1/4] Support multiple css paths * Ensure only one css_path configuration is used * Raise error if length of css_paths does not match routes * Add comment in config file about css_paths --- lib/config/critical_path_css.yml | 3 + lib/critical_path_css/configuration.rb | 4 + lib/critical_path_css/css_fetcher.rb | 9 +- lib/critical_path_css/rails/config_loader.rb | 19 ++- lib/critical_path_css/rails/version.rb | 2 +- .../lib/critical_path_css/css_fetcher_spec.rb | 63 +++++++++ .../rails/config_loader_spec.rb | 125 ++++++++++++++++++ 7 files changed, 219 insertions(+), 6 deletions(-) create mode 100644 spec/lib/critical_path_css/css_fetcher_spec.rb create mode 100644 spec/lib/critical_path_css/rails/config_loader_spec.rb diff --git a/lib/config/critical_path_css.yml b/lib/config/critical_path_css.yml index e916570..5317754 100644 --- a/lib/config/critical_path_css.yml +++ b/lib/config/critical_path_css.yml @@ -3,6 +3,9 @@ defaults: &defaults manifest_name: application # Else provide the relative path of your CSS file from the /public directory # css_path: /path/to/css/from/public/main.css + # Or provide a separate path for each route + # css_paths: + # - /path/to/css/from/public/main.css routes: - / diff --git a/lib/critical_path_css/configuration.rb b/lib/critical_path_css/configuration.rb index b46669c..b8865ff 100644 --- a/lib/critical_path_css/configuration.rb +++ b/lib/critical_path_css/configuration.rb @@ -13,6 +13,10 @@ def css_path @config['css_path'] end + def css_paths + @config['css_paths'] + end + def manifest_name @config['manifest_name'] end diff --git a/lib/critical_path_css/css_fetcher.rb b/lib/critical_path_css/css_fetcher.rb index 680b7c9..3ead1a2 100644 --- a/lib/critical_path_css/css_fetcher.rb +++ b/lib/critical_path_css/css_fetcher.rb @@ -10,7 +10,10 @@ def initialize(config) end def fetch - @config.routes.map { |route| [route, css_for_route(route)] }.to_h + @config.routes.map.with_index { |route, index| + css_path = @config.css_paths[index].present? ? @config.css_paths[index] : @config.css_path + [route, css_for_route(route, css_path)] + }.to_h end def fetch_route(route) @@ -19,10 +22,10 @@ def fetch_route(route) protected - def css_for_route(route) + def css_for_route(route, css_path) options = { 'url' => @config.base_url + route, - 'css' => @config.css_path, + 'css' => css_path, ## optional params # viewport dimensions 'width' => 1300, diff --git a/lib/critical_path_css/rails/config_loader.rb b/lib/critical_path_css/rails/config_loader.rb index bbab1ba..ee4d00d 100644 --- a/lib/critical_path_css/rails/config_loader.rb +++ b/lib/critical_path_css/rails/config_loader.rb @@ -5,12 +5,19 @@ class ConfigLoader def load config = YAML.safe_load(ERB.new(File.read(configuration_file_path)).result, [], [], true)[::Rails.env] - config['css_path'] = "#{::Rails.root}/public" + ( + validate_css_path config + if config['css_path'] + config['css_path'] = "#{::Rails.root}/public" + ( config['css_path'] || ActionController::Base.helpers.stylesheet_path( config['manifest_name'], host: '' ) - ) + ) + config['css_paths'] = [] + else + config['css_path'] = '' + config['css_paths'] = config['css_paths'].collect { |path| "#{::Rails.root}/public#{path}" } + end config end @@ -19,6 +26,14 @@ def load def configuration_file_path @configuration_file_path ||= ::Rails.root.join('config', CONFIGURATION_FILENAME) end + + def validate_css_path(config) + if config['css_path'] && config['css_paths'] + raise LoadError, 'Cannot specify both css_path and css_paths' + elsif config['css_paths'] && config['css_paths'].length != config['routes'].length + raise LoadError, 'Must specify css_paths for each route' + end + end end end end diff --git a/lib/critical_path_css/rails/version.rb b/lib/critical_path_css/rails/version.rb index ebfb91a..58e0b8b 100644 --- a/lib/critical_path_css/rails/version.rb +++ b/lib/critical_path_css/rails/version.rb @@ -1,5 +1,5 @@ module CriticalPathCSS module Rails - VERSION = '2.6.0'.freeze + VERSION = '2.7.0'.freeze end end diff --git a/spec/lib/critical_path_css/css_fetcher_spec.rb b/spec/lib/critical_path_css/css_fetcher_spec.rb new file mode 100644 index 0000000..86e4d89 --- /dev/null +++ b/spec/lib/critical_path_css/css_fetcher_spec.rb @@ -0,0 +1,63 @@ +require 'spec_helper' + +RSpec.describe 'CssFetcher' do + describe '#fetch' do + let(:subject) { CriticalPathCss::CssFetcher.new(config) } + let(:response) { + ['foo','', OpenStruct.new(exitstatus: 0)] + } + let(:routes) { ['/', '/new_route'] } + let(:config) do + OpenStruct.new( + base_url: 'http://0.0.0.0:9292', + css_path: css_path, + css_paths: css_paths, + penthouse_options: {}, + routes: routes + ) + end + + context 'when a single css_path is configured' do + let(:css_path) { '/test.css' } + let(:css_paths) { [] } + + it 'generates css for each route from the same file' do + expect(Open3).to receive(:capture3) do |arg1, arg2, arg3| + options = JSON.parse(arg3) + expect(options['css']).to eq '/test.css' + end.twice.and_return(response) + subject.fetch + end + end + + context 'when multiple css_paths are configured' do + let(:css_path) { '' } + let(:css_paths) { ['/test.css', '/test2.css'] } + + it 'generates css for each route from the respective file' do + expect(Open3).to receive(:capture3) do |arg1, arg2, arg3| + options = JSON.parse(arg3) + expect(options['css']).to eq '/test.css' if options['url'] == 'http://0.0.0.0:9292/' + expect(options['css']).to eq '/test2.css' if options['url'] == 'http://0.0.0.0:9292/new_route' + end.twice.and_return(response) + subject.fetch + end + end + + context 'when same css file applies to multiple routes' do + let(:css_path) { '' } + let(:css_paths) { ['/test.css', '/test2.css', '/test.css'] } + let(:routes) { ['/', '/new_route', '/newer_route'] } + + it 'generates css for each route from the respective file' do + expect(Open3).to receive(:capture3) do |arg1, arg2, arg3| + options = JSON.parse(arg3) + expect(options['css']).to eq '/test.css' if options['url'] == 'http://0.0.0.0:9292/' + expect(options['css']).to eq '/test2.css' if options['url'] == 'http://0.0.0.0:9292/new_route' + expect(options['css']).to eq '/test.css' if options['url'] == 'http://0.0.0.0:9292/newer_route' + end.thrice.and_return(response) + subject.fetch + end + end + end +end diff --git a/spec/lib/critical_path_css/rails/config_loader_spec.rb b/spec/lib/critical_path_css/rails/config_loader_spec.rb new file mode 100644 index 0000000..232b7ab --- /dev/null +++ b/spec/lib/critical_path_css/rails/config_loader_spec.rb @@ -0,0 +1,125 @@ +require 'spec_helper' + +RSpec.describe 'ConfigLoader' do + let(:subject) { CriticalPathCss::Rails::ConfigLoader.new } + describe '#load' do + before do + allow(File).to receive(:read).and_return(config_file) + end + + context 'when single css_path is specified' do + let(:config_file) { + <<~CONFIG + defaults: &defaults + base_url: http://0.0.0.0:9292 + css_path: /test.css + routes: + - / + + development: + <<: *defaults + + test: + <<: *defaults + CONFIG + } + + it 'sets css_path with the path' do + config = subject.load + + expect(config['css_path']).to eq '/app/spec/internal/public/test.css' + end + + it 'leaves css_paths empty' do + config = subject.load + + expect(config['css_paths']).to eq [] + end + end + + context 'when multiple css_paths are specified' do + let(:config_file) { + <<~CONFIG + defaults: &defaults + base_url: http://0.0.0.0:9292 + css_paths: + - /test.css + - /test2.css + routes: + - / + - /new_route + + development: + <<: *defaults + + test: + <<: *defaults + CONFIG + } + + it 'sets css_path to empty string' do + config = subject.load + + expect(config['css_path']).to eq '' + end + + it 'leaves css_paths to an array of paths' do + config = subject.load + + expect(config['css_paths']).to eq ['/app/spec/internal/public/test.css','/app/spec/internal/public/test2.css'] + end + end + + context 'when single css_path and multiple css_paths are both specified' do + let(:config_file) { + <<~CONFIG + defaults: &defaults + base_url: http://0.0.0.0:9292 + css_path: /test.css + css_paths: + - /test.css + - /test2.css + routes: + - / + - /new_route + + development: + <<: *defaults + + test: + <<: *defaults + CONFIG + } + + it 'raises an error' do + expect { subject.load }.to raise_error LoadError, 'Cannot specify both css_path and css_paths' + end + end + + context 'when css_paths and routes are not the same length' do + let(:config_file) { + <<~CONFIG + defaults: &defaults + base_url: http://0.0.0.0:9292 + css_paths: + - /test.css + - /test2.css + routes: + - / + - /new_route + - /newer_route + + development: + <<: *defaults + + test: + <<: *defaults + CONFIG + } + + it 'raises an error' do + expect { subject.load }.to raise_error LoadError, 'Must specify css_paths for each route' + end + end + end +end From 667f2428483d3dd9d4ee3afc0e32bbd954ffa077 Mon Sep 17 00:00:00 2001 From: "Randall Reed, Jr" Date: Mon, 25 Jun 2018 13:35:32 -0400 Subject: [PATCH 2/4] Add note to readme about puppeteer --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 9b354fb..82960fc 100644 --- a/README.md +++ b/README.md @@ -146,6 +146,12 @@ This gem is to be tested inside of docker/docker-compose. [Combustion](https://g Once shell'd in, run `bundle exec rspec spec` to run the test. The test rails app lives in `spec/internal`, and it can be viewed locally at `http://localhost:9292/` +If you encounter Chromium errors trying to run the tests, installing [Puppeteer](https://github.com/GoogleChrome/puppeteer) might help. + +```Bash + npm install puppeteer +``` + ## Versions From 0682adf46db2c3c8ef1fc2ff528ffc94e83be70c Mon Sep 17 00:00:00 2001 From: "Randall Reed, Jr" Date: Mon, 25 Jun 2018 13:36:30 -0400 Subject: [PATCH 3/4] Add page h1 tag to more clearly identify test page --- spec/internal/app/views/root/index.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/internal/app/views/root/index.html.erb b/spec/internal/app/views/root/index.html.erb index 62b320d..6103952 100644 --- a/spec/internal/app/views/root/index.html.erb +++ b/spec/internal/app/views/root/index.html.erb @@ -1 +1,2 @@ +

Critical Path CSS Rails Test App

<%= CriticalPathCss.fetch(request.path) %>

From 90f7fa4f7784dcb304bbbcfe7cfe408167157245 Mon Sep 17 00:00:00 2001 From: "Randall Reed, Jr" Date: Tue, 14 Aug 2018 09:42:53 -0400 Subject: [PATCH 4/4] Add to Usage section of README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 82960fc..c9e0353 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,7 @@ First, you'll need to configue a few things in the YAML file: `config/critical_p * `manifest_name`: If you're using the asset pipeline, add the manifest name. * `css_path`: If you're not using the asset pipeline, you'll need to define the path to the application's main CSS. The gem assumes your CSS lives in `RAILS_ROOT/public`. If your main CSS file is in `RAILS_ROOT/public/assets/main.css`, you would set the variable to `/assets/main.css`. +* `css_paths`: If you have the need to specify multiple CSS source files, you can do so with `css_paths`. Note that `css_path` and `css_paths` are **mutually exclusive**; if using `css_path`, configuration for `css_paths` should be omitted, and vice versa. When using this option, a separate CSS path must be specified for each route, and they will be matched based on the order specified (the first CSS path will be applied to the first route, the second CSS path to the second route, etc). * `routes`: List the routes that you would like to generate the critical CSS for. (i.e. /resources, /resources/show/1, etc.) * `base_url`: Add your application's URL for the necessary environments.