Webpacker support #3414

mshibuya · 2021-11-14T10:42:25Z

coveralls · 2021-11-14T14:01:38Z

Pull Request Test Coverage Report for Build 1459001873

0 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-1.8%) to 94.081%

Files with Coverage Reduction	New Missed Lines	%
app/controllers/rails_admin/application_controller.rb	4	88.89%

Totals
Change from base Build 1415561078:	-1.8%
Covered Lines:	3354
Relevant Lines:	3565

💛 - Coveralls

coveralls · 2021-11-14T14:01:39Z

Pull Request Test Coverage Report for Build 1538205393

45 of 56 (80.36%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 95.987%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/rails_admin/support/esmodule_preprocessor.rb	15	16	93.75%
app/helpers/rails_admin/application_helper.rb	2	5	40.0%
lib/generators/rails_admin/install_generator.rb	21	28	75.0%

Totals
Change from base Build 1533376790:	0.1%
Covered Lines:	3636
Relevant Lines:	3788

💛 - Coveralls

TheBrockEllis · 2021-11-27T19:13:58Z

I realize that this is a draft PR but I want to share my experience trying to install rails_admin from this branch. I ran into issues with the location of my javascript. In my project, I renamed the location of my scripts folders to app/frontend/packs/ based on some popular Rails 6 tutorials. I got the following error when trying to access /admin:

 Field 'browser' doesn't contain a valid alias configuration
              /path/to/rails/project/app/frontend/js/rails_admin/rails_admin.jpg doesn't exist
            as directory
              /path/to/rails/project/app/frontend/js/rails_admin/rails_admin doesn't exist

mshibuya · 2021-11-28T04:54:17Z

@TheBrockEllis Thanks for trying this out, looks like a configuration issue. If you could provide me the source code with relevant configuration I can take a look.

Because rails-ujs has the same functionality now. https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/actionview/app/assets/javascripts/rails-ujs/features/remote.coffee#L36-L37

mshibuya · 2021-11-28T11:02:30Z

@codealchemy @jdufresne
This is going to be a big change, do you guys have time to take a look?
Any suggestions and questions will be appreciated.

jdufresne

Nice work! This is an exciting change.

jdufresne · 2021-11-28T20:21:45Z

.github/workflows/test.yml

      - name: Run check
-        run: npx prettier --check .
+        run: ./node_modules/.bin/prettier --check .


I believe yarn will execute scripts from node_modules/.bin

Suggested change

run: ./node_modules/.bin/prettier --check .

run: yarn prettier --check .

jdufresne · 2021-11-28T20:26:06Z

.github/workflows/test.yml

@@ -12,35 +12,48 @@ jobs:
        gemfile: [gemfiles/rails_6.1.gemfile]
        orm: [active_record]
        adapter: [sqlite3]
+        asset: [sprockets]


Can you explain why aren't other asset pipelines defined here? Don't we want to verify webpack/webpacker in all scenarios?

If we go this way,

ruby: [2.6, 2.7, 3.0, jruby] asset: [sprockets, webpacker]

we'll get every combination of 2.6/sprockets, 2.6/webpacker, 2.7/sprockets, ... and so on. That's kind of redundant, since in general Ruby version will not affect how JavaScript assets are executed.

So I didn't add webpacker in the global matrix, and added it individually like this to save CI execution time and computer resource.

include: ... - ruby: 3.0 gemfile: gemfiles/rails_6.1.gemfile orm: active_record adapter: postgresql asset: webpacker - ruby: 3.0 gemfile: gemfiles/rails_7.0.gemfile orm: active_record adapter: sqlite3 asset: webpacker

jdufresne · 2021-11-28T20:30:24Z

app/helpers/rails_admin/application_helper.rb

@@ -195,5 +195,17 @@ def flash_alert_class(flash_key)
      else "alert-#{flash_key}"
      end
    end
+
+    def handle_asset_dependency_error


Is the existing exception insufficient or confusing? I'm thinking maybe we don't need this.

It gives an exception like this:

LoadError in RailsAdmin::MainController#dashboard cannot load such file -- sassc

not so confusing, but we're dropping existing sassc dependency from our gem so it would be better to be friendly for existing users to instruct what's happening after their gem upgrade.

jdufresne · 2021-11-28T20:33:49Z

app/views/layouts/rails_admin/_head.html.erb

+<% when :webpack, :sprockets %>
+  <% handle_asset_dependency_error do %>
+    <%= stylesheet_link_tag "rails_admin.css", media: :all %>
+    <%= javascript_include_tag "rails_admin.js", defer: true %>


It is better to use async rather than defer.

Can we also apply the aync attribute to the webpacker file?

jdufresne · 2021-11-28T20:35:16Z

gemfiles/rails_6.1.gemfile

  gem "database_cleaner-active_record", ">= 2.0", require: false
+  gem "database_cleaner-mongoid", ">= 2.0", require: false


What are these unrelated ordering changes? Can they be reverted or moved to a separete PR so this remains focused on just webpack and assets?

jdufresne · 2021-11-28T20:46:27Z

package.json

+  ],
+  "main": "src/rails_admin/base.js",
+  "scripts": {
+    "link": "yarn link && cd spec/dummy_app && yarn link rails_admin",


Does this link script get used? I don't see it and am not familiar with it.

It is useful for working with webpack-built assets. After your run yarn run link a symlink is created under dummy_app's node_modules so the changes made to the files under src dir will be immediately picked up by webpack-dev-server.
https://classic.yarnpkg.com/en/docs/cli/link/

jdufresne · 2021-11-28T20:46:56Z

vendor/assets/javascripts/rails_admin/jquery3.js

@@ -0,0 +1,10872 @@
+/*!


Same thought here, could import from node_modules to avoid vendoring?

Same as #3414 (comment).

jdufresne · 2021-11-28T20:48:01Z

.rubocop.yml

    - "spec/dummy_app/bin/**/*"
    - "spec/dummy_app/db/schema.rb"
    - "spec/dummy_app/tmp/**/*"
    - "vendor/bundle/**/*"
  NewCops: disable
+  SuggestExtensions: false


What is this silencing? Maybe it would be better to install the suggestion.

Showing the suggestion everytime Rubocop is run is pointless. I run it so many times during development.
You can install those extensions if you want to, regardless of the suggestion.

jdufresne · 2021-11-28T20:49:34Z

.prettierignore

-app/assets/stylesheets/rails_admin/aristo
-app/assets/stylesheets/rails_admin/themes/cerulean
+coverage
+lib/generators/rails_admin/templates


Why ignore this directory? Sems like this should be formatted the same as any other file.

There's reason for it...

Some of files contained here (specifically speaking, they're environment.js and webpack.config.js) are need to be merged with existing ones which are created by Webpacker or Rails. And if a user already has some configurations, they need to be merged manually with the changes RailsAdmin need.

If we apply prettier to our templates and run RailsAdmin installer, what users get is like this:

% rails g rails_admin:install ... conflict webpack.config.js Overwrite /path/to/webpack.config.js? (enter "h" for help) [Ynaqdhm] d --- /path/to/webpack.config.js 2021-12-04 13:53:59.000000000 +0900 +++ /path/to/webpack.config.js20211204-51180-i5ghjr 2021-12-04 13:54:22.000000000 +0900 @@ -1,18 +1,32 @@ -const path = require("path") -const webpack = require('webpack') +const path = require("path"); +const webpack = require("webpack"); +const MiniCssExtractPlugin = require("mini-css-extract-plugin"); ...

those diffs are unnecessary if we don't apply prettier. Then it looks like:

conflict webpack.config.js Overwrite /path/to/webpack.config.js? (enter "h" for help) [Ynaqdhm] d --- /path/to/webpack.config.js 2021-12-04 13:53:59.000000000 +0900 +++ /path/to/webpack.config.js20211204-51508-jf8k2a 2021-12-04 13:58:12.000000000 +0900 @@ -1,18 +1,32 @@ const path = require("path") const webpack = require('webpack') +const MiniCssExtractPlugin = require("mini-css-extract-plugin") ...

much cleaner.
My intention is enabling users to install RailsAdmin with less work, instead of applying prettier to templates to get feeling of tidiness. Users can choose to apply prettier to their source code anyway, without hustle.

jdufresne · 2021-11-28T20:53:55Z

lib/rails_admin/config.rb

@@ -86,6 +86,9 @@ class << self
      attr_accessor :navigation_static_links
      attr_accessor :navigation_static_label

+      # Set where RailsAdmin fetches JS/CSS from, defaults to :sprockets


As this is targeting a new major release, any thought on defaulting to webpacker? Upstream Rails recommends webpack for new projects and describes sprockets as "legacy": https://edgeguides.rubyonrails.org/webpacker.html

We need to ensure compatibility with existing applications. Since using :webpacker as default value for asset_source doesn't automatically make those applications Webpacker-ready, we need to provide migration path for users.

In this way, users can proceed like this:

Upgrade RailsAdmin, keeping the app functional since RailsAdmin delivers assets using Sprockets by default

Make their app Webpacker-ready

When things are ready, they can configure RailsAdmin to use Webpacker-sourced assets

- Use `yarn run`, instead of `./node_modules/.bin/` - Remove unnecessary .prettierignore entries - Address Rubocop Performance/RegexpMatch offense in the PR build - Apply async: true to javascript tags - Restore order changes in gemfiles/* - Change scripts task `prettier` to `format`, since it conflicts with the prettier command itself - Apply prettier to src/rails_admin/styles/themes/cerulean

mshibuya · 2021-12-04T05:58:02Z

@jdufresne Addressed all review suggestions, please take a look again 🙏

mshibuya · 2021-12-11T03:11:14Z

Let me merge this for now, since it's blocking other works.
If you have any comments please leave them here, I will work on them later 🙏

It was introduced by #3414, but is not fully functional yet

mshibuya force-pushed the webpacker branch 2 times, most recently from ffa7d8a to b383d35 Compare November 14, 2021 13:46

mshibuya force-pushed the webpacker branch 3 times, most recently from 612cd6f to ee102ce Compare November 20, 2021 10:23

mshibuya force-pushed the webpacker branch from ee102ce to 46eb3de Compare November 28, 2021 08:47

mshibuya marked this pull request as ready for review November 28, 2021 09:44

mshibuya added 8 commits November 28, 2021 18:45

Use Ruby3 compatible version of mlb

4cfecb6

Remove remotipart dependency

58b76d1

Because rails-ujs has the same functionality now. https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/actionview/app/assets/javascripts/rails-ujs/features/remote.coffee#L36-L37

Remove bootstrap-sass code, which is no longer relevant

1dc6c3d

Add ability to test with Webpacker-built assets

f34f631

Migrate assets to ESM-based structure

c5fe17a

Replace images with Font Awesome icons

a0a568b

Add back support for Sprockets-based asset pipeline

7918dc7

Add installer support for webpack/webpacker

b84d065

mshibuya force-pushed the webpacker branch from 46eb3de to b84d065 Compare November 28, 2021 09:46

jdufresne reviewed Nov 28, 2021

View reviewed changes

mshibuya force-pushed the webpacker branch from 7976e47 to a900385 Compare December 4, 2021 05:38

mshibuya merged commit 28cee7f into master Dec 11, 2021

mshibuya deleted the webpacker branch December 11, 2021 03:11

mshibuya mentioned this pull request Dec 21, 2021

assets:precompile results in LoadError: cannot load such file -- sassc #3450

Closed

mshibuya mentioned this pull request Feb 12, 2022

3.0.0.rc causes issues with webpacker #3467

Closed

mshibuya mentioned this pull request Feb 12, 2022

Custom theme overrides not working on beta releases #3466

Closed

mshibuya added a commit that referenced this pull request Feb 27, 2022

Remove webpack auto detection from the installer

3dae7f3

It was introduced by #3414, but is not fully functional yet

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webpacker support #3414

Webpacker support #3414

mshibuya commented Nov 14, 2021

coveralls commented Nov 14, 2021

coveralls commented Nov 14, 2021 •

edited

Loading

TheBrockEllis commented Nov 27, 2021

mshibuya commented Nov 28, 2021

mshibuya commented Nov 28, 2021

jdufresne left a comment

jdufresne Nov 28, 2021

jdufresne Nov 28, 2021

mshibuya Dec 1, 2021

jdufresne Nov 28, 2021

mshibuya Dec 1, 2021

jdufresne Nov 28, 2021

jdufresne Nov 28, 2021

jdufresne Nov 28, 2021

mshibuya Dec 4, 2021

jdufresne Nov 28, 2021

mshibuya Dec 4, 2021

jdufresne Nov 28, 2021

mshibuya Dec 4, 2021

jdufresne Nov 28, 2021

mshibuya Dec 4, 2021

jdufresne Nov 28, 2021

mshibuya Dec 4, 2021

mshibuya commented Dec 4, 2021

mshibuya commented Dec 11, 2021

	run: ./node_modules/.bin/prettier --check .
	run: yarn prettier --check .

		gem "database_cleaner-active_record", ">= 2.0", require: false
		gem "database_cleaner-mongoid", ">= 2.0", require: false

Webpacker support #3414

Webpacker support #3414

Conversation

mshibuya commented Nov 14, 2021

coveralls commented Nov 14, 2021

Pull Request Test Coverage Report for Build 1459001873

💛 - Coveralls

coveralls commented Nov 14, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1538205393

💛 - Coveralls

TheBrockEllis commented Nov 27, 2021

mshibuya commented Nov 28, 2021

mshibuya commented Nov 28, 2021

jdufresne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshibuya commented Dec 4, 2021

mshibuya commented Dec 11, 2021

coveralls commented Nov 14, 2021 •

edited

Loading