Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions lib/config/critical_path_css.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
- /

Expand Down
4 changes: 4 additions & 0 deletions lib/critical_path_css/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions lib/critical_path_css/css_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand Down
19 changes: 17 additions & 2 deletions lib/critical_path_css/rails/config_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
2 changes: 1 addition & 1 deletion lib/critical_path_css/rails/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module CriticalPathCSS
module Rails
VERSION = '2.6.0'.freeze
VERSION = '2.7.0'.freeze
end
end
1 change: 1 addition & 0 deletions spec/internal/app/views/root/index.html.erb
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
<h1>Critical Path CSS Rails Test App</h1>
<p><%= CriticalPathCss.fetch(request.path) %></p>
63 changes: 63 additions & 0 deletions spec/lib/critical_path_css/css_fetcher_spec.rb
Original file line number Diff line number Diff line change
@@ -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
125 changes: 125 additions & 0 deletions spec/lib/critical_path_css/rails/config_loader_spec.rb
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@randallreedjr, Is it possible to move this config into a fixture of sorts? I think it may help improve the readability and clean up this spec if we were able to write something like let(:config_file) { File.open('/path/to/single_css_path_conf.yml') } or something along those lines. Not sure if a fixture is the right call or just moving the config into a file in a config directory. Likewise, the same can be done with the other similar config variables.

Once this is address, I'll go ahead and merge this in and tag you with the merge request that has the refactors in it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@randallreedjr Can you see my above message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-misshore Yep, will do!

<<~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