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

Remove bin/heroku_install #544

Merged
merged 1 commit into from
Aug 4, 2017
Merged

Remove bin/heroku_install #544

merged 1 commit into from
Aug 4, 2017

Conversation

seanpdoyle
Copy link
Contributor

For deployments to Heroku, depend on rake ember:{build,install} being
executed as part of rake assets:precompile.

The removal of bin/heroku_install as part of a postinstall script in
the root-level package.json simplifies support as well.

This commit modifies the setup to incorporate checks for the presence of
bower.json to determine whether or not to cache the corresponding
bower_components directories, as well as the up-front installation of
the bower CLI tool.

end

def stub_apps(applications)
allow(EmberCli).to receive(:apps).and_return(applications)

Choose a reason for hiding this comment

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

Use 2 (not 4) spaces for indentation.

it "delegates to the collection of applications" do
stub_apps(
app_with_foo: { foo: true },
app_without_foo: { foo: false},

Choose a reason for hiding this comment

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

Space inside } missing.

if bower_enabled
cacheable_directories = [OUTPUT_DIRECTORY.join("bower_components")]
else
cacheable_directories =[]

Choose a reason for hiding this comment

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

Useless assignment to variable - cacheable_directories.
Surrounding space missing for operator =.


def cacheable_directories(bower_enabled)
if bower_enabled
cacheable_directories = [OUTPUT_DIRECTORY.join("bower_components")]

Choose a reason for hiding this comment

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

Useless assignment to variable - cacheable_directories.


allow_any_instance_of(EmberCli::App).
to receive(:cacheable_directories).and_return(
cacheable_directories(bower_enabled)

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

lib/ember_cli.rb Outdated
@@ -39,6 +39,10 @@ def skip?
ENV["SKIP_EMBER"].present?
end

def any?(*arguments, &block)

Choose a reason for hiding this comment

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

Method EmberCli#any? is defined at both lib/ember_cli.rb:34 and lib/ember_cli.rb:42.

For deployments to Heroku, depend on `rake ember:{build,install}` being
executed as part of `rake assets:precompile`.

The removal of `bin/heroku_install` as part of a `postinstall` script in
the root-level `package.json` simplifies support as well.

This commit modifies the setup to incorporate checks for the presence of
`bower.json` to determine whether or not to cache the corresponding
`bower_components` directories, as well as the up-front installation of
the `bower` CLI tool.
@seanpdoyle seanpdoyle merged commit 989e610 into master Aug 4, 2017
@seanpdoyle seanpdoyle deleted the install-with-rake branch August 4, 2017 16:00
@nruth
Copy link
Contributor

nruth commented Sep 17, 2017

Getting rid of heroku_install seems like a good change because it's easy to forget to update.
However, this seems to stop heroku from caching the downloaded packages. This doesn't seem worth fixing, but thought I'd point it out.

When the NPM buildpack runs first and caches there's nothing installed yet so nothing to cache, and when the Ruby buildpack (Rails 4.2) runs it only looks for gems and doesn't seem to have customisable cache paths.

I guess it could be addressed by adding custom cache paths to the ruby buildpack, or by adding generic buildpacks that can capture and restore custom cache paths at the start and end of the buildpack stack.

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.

3 participants