Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ember CLI Rails Loader #34

Merged
merged 21 commits into from
May 18, 2017

Conversation

djones
Copy link
Contributor

@djones djones commented May 17, 2017

Adds support for loading in assets from both Ember and Rails when using Ember CLI Rails

Configuration

In your spec_helper.rb file you need to configure use of this loader like so:

RSpec.configure do |config|
  # ...
  config.before(:suite) do
    # Ember CLI must compile first to create all the required files.
    EmberCli.compile!

    # You need to tell the loader which apps are mounted on which paths.
    ember_apps = { frontend: '/', admin: '/admin/' }

    Percy::Capybara.use_loader(:ember_cli_rails, { mounted_apps: ember_apps})

    Percy::Capybara.initialize_build
  end

  config.after(:suite) { Percy::Capybara.finalize_build }
end

You would have the above configuration if you have something like this in your routes.rb file:

Rails.application.routes.draw do
  # Useful for just subbing in a vanilla Rails controller for testing.
  # root to: 'pages#index'

  mount_ember_app :admin, to: "/admin", controller: "admin"
  mount_ember_app :frontend, to: "/"
end

Example App

percy/example-ember-cli-rails#1

Todo

  • write tests for Percy::Capybara::Loaders::EmberCliRailsLoader

@djones djones requested a review from fotinakis May 17, 2017 23:58
Copy link
Contributor

@fotinakis fotinakis left a comment

Choose a reason for hiding this comment

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

@djones Nice work, this looks excellent.

Two tiny comments above. Also, whatever happened to the EmberCli.compile! step?

next if File.size(path) > MAX_FILESIZE_BYTES

# Replace the assets_dir with the base_url to generate the resource_url
resource_url = File.join(base_url, path.sub(root_path.to_s, ''))
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work on Windows because this is a URL and File.join uses the OS path separator instead of always forward slashes, so you'll get \main.css. Need to find a URL joiner or use the old join code.

@@ -109,6 +116,28 @@ def iframes_resources
rescue ::Capybara::NotSupportedByDriverError
[]
end

def _resources_from_path(root_path, base_url: '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: could you rename this to _resources_from_dir and root_dir arg for clarity / slightly more consistency with some other places in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fotinakis cool, will do that.

@fotinakis
Copy link
Contributor

Ahh I see that EmberCli.compile! is a manual setup step outside. That's 👍 for now.

@djones
Copy link
Contributor Author

djones commented May 18, 2017

@fotinakis yeah, and honestly I think that is the right approach anyway. Feels weird for the loader to fire off asset compilation.

@djones djones changed the title WIP: Add Ember CLI Rails Loader Add Ember CLI Rails Loader May 18, 2017
Copy link
Contributor

@fotinakis fotinakis left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰 🎉

@djones djones merged commit 3b41c75 into master May 18, 2017
@djones djones deleted the david_jones/ch385/fix-ember-cli-rails-asset-discovery branch May 18, 2017 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants