Conversation
features/support/env.rb
Outdated
|
|
||
| # Force deprecations to raise an exception. | ||
| ActiveSupport::Deprecation.behavior = :raise | ||
| ActiveSupport::Deprecation.behavior = ActiveAdmin::Dependency.rails?('< 5.1.x') ? :raise : :stderr # FIXME: need database_cleaner 1.6.0. |
There was a problem hiding this comment.
@varyonic , can we change Gemfile to use database_cleaner 1.6.0. from github ?
There was a problem hiding this comment.
1.6.0 is not released yet. We can revert Gemfile to gem 'database_cleaner', github: 'DatabaseCleaner/database_cleaner' as it was last summer after Rails 5.0.
There was a problem hiding this comment.
@varyonic , basically I'm ok with both options, but we should somehow not forget to return ActiveSupport::Deprecation.behavior
| When I am on the index page for posts | ||
| Then I should see the css file "active_admin.css" | ||
| Then I should see the js file "active_admin.js" | ||
| Then I should see the css file "active_admin" |
There was a problem hiding this comment.
@varyonic , would you please explain why we remove file extension here?
There was a problem hiding this comment.
Because I removed asset.digest = false, so the css file is now something like active_admin.87a9...98ef.css As of 5.1 I believe digesting is done by Webpack and digest = false seems ignored.
There was a problem hiding this comment.
Since our rough conclusion was that webpacker is opt-in and we're not using it, I have the feeling that the old setup might actually work. Should we double check this?
activeadmin.gemspec
Outdated
| s.add_dependency 'railties', '>= 4.0', '< 5.1' | ||
| s.add_dependency 'ransack', '~> 1.3' | ||
| s.add_dependency 'sass-rails' | ||
| s.add_dependency 'thor', '0.19.1', '!=0.19.2', '!=0.19.3', '!=0.19.4' # http://stackoverflow.com/questions/41207432/ |
There was a problem hiding this comment.
'0.19.1', '!=0.19.2', '!=0.19.3', '!=0.19.4' doesn't make sense.
'~>0.19.1', '!=0.19.2', '!=0.19.3', '!=0.19.4', would make sense.
Be in general I think we should not fix third party bugs. We only should limiting version if our code has a problem with this version, not if JBuilder has a problem with this version.
There was a problem hiding this comment.
Is jbuilder a direct runtime dependency of AA or does this fix only a problem in our tests? If the latter, the dependency should be added to the Gemfile, or even better, to lock files committed to version control.
There was a problem hiding this comment.
Only impacts rake:setup and is an unclear warning rather than an error. I'm inclined to omit this.
There was a problem hiding this comment.
Yeah, let's do that. I need to find time to propose the addition of lock files to the repo, but it definitely doesn't belong here.
|
Rebased and updated for review. |
|
@timoschilling Shall I add to CHANGELOG? |
.travis.yml
Outdated
|
|
||
| before_install: | ||
| - rm -rf ~/.nvm && git clone https://github.com/creationix/nvm.git ~/.nvm && (cd ~/.nvm && git checkout `git describe --abbrev=0 --tags`) && source ~/.nvm/nvm.sh && nvm install 4 | ||
| - npm install -g yarn |
There was a problem hiding this comment.
Do we need nvm and yarn in CI now?
There was a problem hiding this comment.
Yes, asset compilation will fail without it as they are prerequistes for webpacker.
There was a problem hiding this comment.
But I thought this was an opt-in feature and I don't see the webpacker gem specified anywhere...
In any case, I still don't understand why we need nvm and why we use it to install... nodejs v4 (apparently webpacker prerrequiste is nodejs v6.4.0+).
There was a problem hiding this comment.
The beta release note is not entirely clear, it states webpack is optional but yarn is required. I ran out of time to dig into why. The (0.12?) nvm pre-installed by Travis is too old. I copied the workaround from a blog post. Feel free to offer a better solution.
There was a problem hiding this comment.
My try would be to remove those two lines, since I don't think they are needed. It's very surprising to me that the build fails without them.
There was a problem hiding this comment.
I believe you, I just don't understand why. Anybody?
There was a problem hiding this comment.
Appears I was wrong. They appeared necessary before but no longer. Will push an update.
…rs ignored. Enable asset digests. As of 5.1 digesting is done by Webpack and digest = false appears ignored.
.gitignore
Outdated
| .rbenv-version | ||
| .localeapp/* | ||
| lib/bug_report_templates/tmp | ||
| gemfiles/bin |
There was a problem hiding this comment.
What about this? I wouldn't expect any binstubs generated here. Maybe it's related to the old setup with yarn and nvm?
…o fix a deprecation warning.
Waiting for database_cleaner 1.6.0 with deprecation warning fixed.