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

Serve EmberCLI tests as mountable engine #90

Merged
merged 1 commit into from
Feb 22, 2015
Merged

Serve EmberCLI tests as mountable engine #90

merged 1 commit into from
Feb 22, 2015

Conversation

seanpdoyle
Copy link
Contributor

Closes #18

Expose EmberCLI test suite through Rails

@seanpdoyle
Copy link
Contributor Author

screen shot 2015-02-22 at 11 03 14 am

Unfortunately, this doesn't yet work.

The app/frontend/tests/index.html is being served properly from the EmberCLI dist_path, but the script and link tags are pointing to assets like assets/vendor.css, instead of assets/frontend/vendor-FINGERPRINT.css.

Any ideas? cc: @rwjblue @rwz @rondale-sc

@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

This is amazing. Thanks for this great work!

We could maybe perform a naive gsub before serving the html to replace assets/ with assets/:app_name/. That's easy and solves the problem.

@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

Also, why remove railties from dependiencies? Rails::Engine is a part of railties as well AFAIK.

@@ -1,4 +1,4 @@
require "ember-cli/railtie" if defined?(Rails)
require "ember-cli/engine" if defined?(Rails)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwz if the railties / engine is only loaded when Rails constant exists, we're really dependent on Rails, not railties.

If Rails is included in the dependencies (which it should be, this being ember-cli-rails and all), then engines and railties come along for the ride, right?

If not, I'll gladly add it back to the gemspec

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rails constant is defined in railties. So in this particular case we're depending on either rails or railties. I don't really have a hard preference on either of those, but one of them definitely should be there.

@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

Also, what do you think about implicitly mounting the engine in development environment? But maybe use less obvious namespace, like /ember-cli-tests/:app_name instead? Then we could save some user's effort setting it up and not be a nuisance to those who don't care about this feature.

@seanpdoyle
Copy link
Contributor Author

I am against implicitly mounting the engine.

What happens if people want to configure the URL?

We could add another configuration value:

EmberCLI.configure do |config|
  config.tests_root = "/my-ember-cli-tests"
end

Or we could just depend on the pre-existing API provided by Rails engines.

For users who don't care about this feature, they're opted out by default, and don't have to worry about our obscure URL convention conflicting with their existing routes.

Also, requiring an explicit mount frees us from worrying about catch-all routes.

# config/routes.rb

# this would have precedent over our route
match "*route-to-ember" => "ember#index"

# this would match before the catchall
mount EmberCLI::Engine => "tests"
match "*route-to-ember" => "ember#index"

@seanpdoyle
Copy link
Contributor Author

It's worth noting that <script src="testem.js"> is 404ing, since it isn't in the asset pipeline.

This doesn't seem to affect the test runnner, though

@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

It seems to be an empty dummy file: https://github.com/ember-cli/ember-cli/blob/v0.1.15/lib/broccoli/testem.js

Seems like we could safely ignore the fact it's missing. The other option would be to add something like this to routes:

get 'testem', to: ->(*){ [200, {}, [""]] }, format: :js

@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

... or even better, handle it in the middleware. :)

Closes #18

Expose EmberCLI test suite through Rails
@seanpdoyle
Copy link
Contributor Author

For the time being, I'm ok with it 404ing.

rwz added a commit that referenced this pull request Feb 22, 2015
Serve EmberCLI tests as mountable engine
@rwz rwz merged commit c4b5500 into thoughtbot:master Feb 22, 2015
@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

Thanks again, you did an awesome job implementing this feature. 👍 👍 👍

Do you want to be a maintainer if the gem? You seem to care and put in a ton of work implementing features. We could definitely use all the help we could get from highly motivated people like you :)

@seanpdoyle seanpdoyle deleted the serve-tests branch February 22, 2015 17:27
@seanpdoyle
Copy link
Contributor Author

@rwz I'd love to!

@rwz
Copy link
Collaborator

rwz commented Feb 22, 2015

@seanpdoyle done.

rwz added a commit that referenced this pull request Feb 22, 2015
@rondale-sc
Copy link
Collaborator

@seanpdoyle @rwz 👍 🍻

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.

Make original ember tests available through rails routes
3 participants