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 webpacker compatibility with config switch and installation generator #5855

Merged
merged 8 commits into from
Mar 19, 2020

Conversation

sgara
Copy link
Contributor

@sgara sgara commented Sep 13, 2019

After reading a few issues and related discussions (see #5829, #5551 and #5048), and willing to switch to webpacker myself, I wanted to propose a first step toward moving to webpacker. This is of course open to discussion. The goal here is to be able to simple switch to loading active_admin assets from webpack instead of sprockets. Also introduced a gemfile to generate development app running webpacker although there is no js nor css at the moment.

To me, the next step would be to create a npm package from sources present in this repo already. Note that a few changes may be required, I'm not sure we can have one set of assets compatible with both webpack and sprockets. But that can be discussed separately I guess

Copy link

@marcelotoledo5000 marcelotoledo5000 left a comment

Choose a reason for hiding this comment

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

Congrats!

@deivid-rodriguez
Copy link
Member

Thanks @sgara! I haven't missed this, I'll have a closer look soon but it looks like a good first initial step in principle!

@sgara
Copy link
Contributor Author

sgara commented Sep 19, 2019

thanks @deivid-rodriguez, no hurry. I'm already taking a look at the next step.
Moving assets to app/javascript, bundling with rollup for still being able to import through sprocket without breaking existing apps and keeping only one set of source files seems to be the way to go. We'll check that in another PR ;)

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Starting to look into this. I added a few comments!

lib/active_admin/namespace_settings.rb Show resolved Hide resolved
gemfiles/rails_60_webpacker.gemfile Outdated Show resolved Hide resolved
spec/support/rails_template.rb Outdated Show resolved Hide resolved
@sgara
Copy link
Contributor Author

sgara commented Nov 13, 2019

Finally getting there!
@deivid-rodriguez @javierjulio PR updated. I added generators and templates to have a seamless testing process, installation instructions, ...
Looking forward to have your comments!

@nyrf
Copy link

nyrf commented Nov 20, 2019

@sgara I got an error

undefined method `html_safe' for nil:NilClass
Did you mean?  html_safe?

WX20191120-163650@2x

@sgara
Copy link
Contributor Author

sgara commented Nov 20, 2019

Thanks for the feedback @nyrf I’ll look into it 👍

@nyrf
Copy link

nyrf commented Nov 21, 2019

@sgara I think that because of style file wouldn't be generated in development environment, so the style file seems like http://localhost:3000/7de125c1-80c7-4e9c-a40a-4a5547107245, i use stylesheet_pack_tag(style, options) || "" and then it works for me.

active_admin_application.stylesheets.each do |style, options|
  stylesheet_tag = active_admin_namespace.use_webpacker ? stylesheet_pack_tag(style, options) || "" : stylesheet_link_tag(style, options)
  text_node(stylesheet_tag.html_safe)
end

@sgara
Copy link
Contributor Author

sgara commented Nov 24, 2019

@nyrf should be fixed now, thanks for testing 👍

@nyrf
Copy link

nyrf commented Nov 27, 2019

@sgara It works. thanks for your work.

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Looks great thanks! Just a suggestion on automating the webpacker environment config too.

docs/0-installation.md Outdated Show resolved Hide resolved
@maokomioko
Copy link

Any chances we can merge it before 2.6 release? =^..^=

@sgara sgara changed the title Add simple config flag to switch to webpacker assets Add webpacker compatibility with config switch and installation generator Dec 17, 2019
@sgara sgara force-pushed the add-webpacker-switch branch 5 times, most recently from 2aed4c4 to 74b3a7d Compare December 17, 2019 20:45
@sgara sgara force-pushed the add-webpacker-switch branch 3 times, most recently from 85e86fc to a1350c0 Compare January 28, 2020 12:31
@sgara
Copy link
Contributor Author

sgara commented Mar 13, 2020

I have to go back into it, it's been such a while now I forgot most of it.
I've rebased the branch and added circleci jobs (nice catch 😆 ), I'll check on your comments

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

@sgara Added a final pass of comments. At this point really minor, I'm almost ready to merge and ship this! 🎉

gemfiles/rails_60_webpacker/Gemfile.lock Show resolved Hide resolved
gemfiles/rails_60_webpacker/Gemfile Outdated Show resolved Hide resolved
spec/support/rails_template.rb Outdated Show resolved Hide resolved
spec/support/rails_template.rb Show resolved Hide resolved
spec/support/rails_template.rb Outdated Show resolved Hide resolved
spec/unit/views/pages/layout_spec.rb Show resolved Hide resolved
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

It looks good to me! Just last tiny comments about the new Gemfile:

  • Can you run BUNDLE_GEMFILE=gemfiles/rails_60_webpacker/Gemfile bundle lock --add-platform java and commit the result? This makes it so that jruby developers don't get warnings when bundling the Gemfile. It will be done by default by the next bundler version, but it's not yet released.

  • Can you pin the rails dependency like it's done in master for the other Gemfiles? This is so that we still get the warnings fixed, but dependabot doesn't create PRs for every commit in Rails.

Other than these minor things, this looks good!

@javierjulio Any comments from your side? Does this look good to you?

@sgara
Copy link
Contributor Author

sgara commented Mar 17, 2020

I’ll make the changes tonight I think. Glad to see the end of the road!

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thank you @sgara for all your work here! It looks great. ❤️ All I have are minor comments. Majority are just text changes. Thanks David!

docs/0-installation.md Outdated Show resolved Hide resolved
lib/generators/active_admin/install/install_generator.rb Outdated Show resolved Hide resolved
spec/requests/stylesheets_spec.rb Show resolved Hide resolved
spec/support/rails_template.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Fix linting issue
@sgara sgara force-pushed the add-webpacker-switch branch 2 times, most recently from 3b8494e to 9355880 Compare March 17, 2020 21:29
sgara added 2 commits March 17, 2020 22:34
Update webpacker Gemfile.lock

Update Gemfile.lock

Update webpacker gemfile to silence ruby 2.7 warnings

Lock jruby dependencies

Lock rails version

Update Gemfile.lock
Use `generate` instead of `rake` for asset setup

Clean duplicated comments

Fix installation generator typo
@sgara sgara force-pushed the add-webpacker-switch branch 2 times, most recently from bf1c2c4 to c7729ae Compare March 17, 2020 22:05
sgara and others added 5 commits March 17, 2020 23:06
Skip asset generation in case of conflicts

Update rails_template.rb

Put back turbolinks setup to previous rails template location
Update installation instructions

Co-Authored-By: David Rodríguez <deivid.rodriguez@riseup.net>
Remove circleci/config.yml trailing whitespace
Co-Authored-By: Javier Julio <javierjulio@icloud.com>
@sgara sgara force-pushed the add-webpacker-switch branch from c7729ae to 8c5ed9d Compare March 17, 2020 22:06
@sgara
Copy link
Contributor Author

sgara commented Mar 17, 2020

Applied changes from both @deivid-rodriguez and @javierjulio reviews and cleaned git history a bit.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Wowowowowoooo 🎉, this is so awesome and sooooo long due, THANK YOU!!!

The changes in the changelog collide with #6133, but I don't mind rebasing my changes after this one, so... 👍 from me!

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Massive effort. Thanks so much @sgara for seeing this through! 👌🏻 We really appreciate it. ❤️

@javierjulio javierjulio merged commit 733a0d0 into activeadmin:master Mar 19, 2020
@deivid-rodriguez
Copy link
Member

Great, I'll be releasing a new version very soon, thanks everyone, specifically @sgara for his massive effort!

@ziaulrehman40
Copy link

Why did we not let users use both webpacker and sprockets at the same time?
It is very possible and used widely as well for some use cases.

If i make a PR to fix this, would that be accepted?

@javierjulio
Copy link
Member

@ziaulrehman40 yes, I'd be interested to see what's involved.

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.

8 participants