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

Make bower an optional dependency #532

Merged
merged 1 commit into from
Jun 9, 2017
Merged

Conversation

seanpdoyle
Copy link
Contributor

Closes #531.

According to bower warning messages, the project is semi-deprecated:

While Bower is maintained, we recommend Yarn and Webpack for *new*
front-end projects! Yarn's advantage is security and reliability, and
Webpack's is support for both CommonJS and AMD projects. Currently
there's no migration path but we hope you'll help us figure out one.

As such, the build process will skip bower install and won't require a
bower if the application's bower.json file is missing.

@mike-burns
Copy link

Words seem right, code seems right.

Copy link

@geoffharcourt geoffharcourt left a comment

Choose a reason for hiding this comment

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

One question. LGTM! 🍖

@@ -63,18 +67,19 @@ def build_error_file
def bower
@bower ||= begin
bower_path = app_options.fetch(:bower_path) { which("bower") }
bower_executable = Pathname(bower_path.to_s)
Copy link

@geoffharcourt geoffharcourt Jun 9, 2017

Choose a reason for hiding this comment

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

I was surprised this works! Is there any notable difference between Pathname.new and Pathname()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember exactly where I saw this syntax, but it came from docs.

It's definitely surprising, I'll replace it with Pathname.new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geoffharcourt I plan to merge #533 ahead of this PR.

@seanpdoyle seanpdoyle force-pushed the conditionally-invoke-bower branch 2 times, most recently from b8d96af to 02afd8f Compare June 9, 2017 20:12
Closes [#531].

According to `bower` warning messages, the project is semi-deprecated:

```
While Bower is maintained, we recommend Yarn and Webpack for *new*
front-end projects! Yarn's advantage is security and reliability, and
Webpack's is support for both CommonJS and AMD projects. Currently
there's no migration path but we hope you'll help us figure out one.
```

As such, the build process will skip `bower install` and won't require a
`bower` if the application's `bower.json` file is missing.

[#531]: #531
@seanpdoyle seanpdoyle merged commit 34ff6dd into master Jun 9, 2017
@seanpdoyle seanpdoyle deleted the conditionally-invoke-bower branch June 9, 2017 20:26
@jdurand
Copy link

jdurand commented Jun 13, 2017

This just broke my Semaphore CI build with the following even though I have bower.json in my ember.js projects.

Missing bower packages: 

[...]

Run `bower install` to install missing dependencies.

Since this is a breaking change, shouldn't it have been part of a minor version bump?

@seanpdoyle
Copy link
Contributor Author

@jdurand could you try CI again, depending on gem "ember-cli-rails", github: "thoughtbot/ember-cli-rails", branch: "revert-34ff6dd" (#535).

If that succeeds, we'll publish a bugfix version for0.8.x, and bring forward 0.8.6's changes to 0.9.0.

@jdurand
Copy link

jdurand commented Jun 13, 2017

Yes, revert-34ff6dd fixes the build.

Note that I was able to fix this by running bower install as a postinstall script:

//package.json
{
  "scripts": {
    "postinstall": "bower install"
  }
}

This could be the 0.9.x way to go as long as it's mentioned in the README and/or CHANGELOG.

seanpdoyle added a commit that referenced this pull request Jun 13, 2017
This reverts commit 34ff6dd, so that it
can be reintroduced as a +  breaking change, [requiring a major version
bump][#532-comment].

[#532-comment]: #532 (comment)
Samsinite pushed a commit to Samsinite/ember-cli-rails that referenced this pull request Mar 10, 2020
This reverts commit 34ff6dd, so that it
can be reintroduced as a +  breaking change, [requiring a major version
bump][tricknotes#532-comment].

[tricknotes#532-comment]: tricknotes#532 (comment)
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.

4 participants