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

Support Importmap and revise Webpack support #3488

Merged
merged 6 commits into from
Jun 19, 2022
Merged

Support Importmap and revise Webpack support #3488

merged 6 commits into from
Jun 19, 2022

Conversation

mshibuya
Copy link
Member

@mshibuya mshibuya commented Mar 2, 2022

This PR adds ability to use Importmap (via importmap-rails) or Webpack (via jsbundling-rails), along with supporting CSS build (via cssbundling-rails) in Rails 7.0 applications.
Existing preliminary support for Webpack was revised in attempt to follow the jsbundling-rails/cssbundling-rails way.
This implementation should also be compatible with propshaft setup, instead of sprockets.

But please note that the Importmap implementation can't be used as it is, since it requires the npm package to be available for CDN delivery. I'll release the npm package soon, possibly as '3.1.0-alpha' or so.
Webpack implementation should work even now, so please try it and give feedback.

end

initializer 'RailsAdmin precompile hook', group: :all, after: 'RailsAdmin apply configuration' do |app|
case RailsAdmin.config.asset_source
Copy link

Choose a reason for hiding this comment

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

@mshibuya
Running rails rails_admin:install will fail. You need to check if the class exists and return an importmap.

def asset_source

I made the following modifications. I hope this will be helpful.

#3495

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the problem, what did you do and what did you get?
Did you try to do config.asset_source = :importmap without installing importmap-rails?

Copy link

@yubele yubele Mar 12, 2022

Choose a reason for hiding this comment

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

Did you try to do config.asset_source = :importmap without installing importmap-rails?

importmap-rails has been pre-installed.

config.asset_source = :importmap

I believe the above refers to RailsAdmin.config.asset_source.

I believe this problem occurs because $ bin/rails rails_admin:install has not yet created config/initializers/rails_admin.rb at the stage of running $ bin/rails rails_admin:install.

Here are the steps I followed

  1. $ bundle add importmap-rails.
    2.$ . /bin/rails importmap:install.
    3.$ bundle add rails_admin --branch importmap --github railsadminteam/rails_admin
  2. $ bin/rails rails_admin:install.

I got the following error in 4.

[Warning] After upgrading RailsAdmin to 3.x you haven't set asset_source yet, using :sprockets as the default.
To suppress this message, run 'rails rails_admin:install' to setup the asset delivery method suitable to you.
/usr/local/bundle/bundler/gems/rails_admin-d073c4dfd315/lib/rails_admin/engine.rb:53:in `block in <class:Engine>': uninitialized constant RailsAdmin::Engine::Sprockets (NameError)

        Sprockets.register_preprocessor 'application/javascript', RailsAdmin::ESModulePreprocessor
        ^^^^^^^^^
Did you mean?  Socket
               Process
               IPSocket

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I understood now, thanks for letting me know. Fixed by 705fd32 👍

@mshibuya mshibuya force-pushed the importmap branch 4 times, most recently from 0929f7b to 7ac5c0c Compare June 13, 2022 14:44
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 95.994% when pulling 425f2e3 on importmap into ce1bb5a on master.

@mshibuya mshibuya marked this pull request as ready for review June 14, 2022 04:44
@mshibuya
Copy link
Member Author

Let me merge this in now, to prepare for the beta release.

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