Project

General

Profile

Actions

Feature #39111

closed

Enable Asset Pipeline Integration using Propshaft

Added by Takashi Kato 7 months ago. Updated 11 days ago.

Status:
Closed
Priority:
Normal
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed

Description

This patch enables the Asset Pipeline in Redmine, assuming you have upgraded to Rails 7(#36320).

Until Rails 6.1, Sprockets was a prerequisite for using the Asset Pipeline. However, starting from Rails 7, Propshaft has been introduced as an alternative option.
While Propshaft offers fewer features compared to Sprockets, it boasts a smaller codebase and greater clarity in its operations (eliminating the need for a JavaScript runtime). Additionally, manifest files are no longer necessary during development.

Key benefits of using Propshaft include:

  • The ability to append file digests to file names instead of Rails3 stytle cache-busting approach involving query strings at the end of assets (expected to enhance cache hit rates).
  • Support for tasks like copying plugin assets to the public directory each time Redmine starts.
  • Compatibility with Turbo, which is almost mandatory in Rails 7 and later. (Note that Rails 7 and later deprecate rails-ujs, the current Redmine dependency.)

To address potential challenges associated with introducing Propshaft, we have extended its functionality in the following ways:

  • We've made it possible for Propshaft to handle Redmine plugin and theme assets without requiring modification(If we use propshaft in default, we will need to modify the links to other assets in stylesheets.).
  • We've ensured that introducing Asset Pipeline does not necessitate rewriting existing stylesheets, thus avoiding major incompatibilities with Redmine themes.
  • In a production environment, rather than manually running the rails asset:precompile command before starting Rails, we now automatically check for asset updates and run precompilation as needed. (You can disable this automatic execution by setting config.assets.redmine_detect_update to false in config/environments/production.rb).

Files


Related issues

Related to Redmine - Patch #40137: Jstoolbar help files should import images from the asset pipelineClosedMarius BĂLTEANU

Actions
Related to Redmine - Defect #40204: `rake redmine:plugins` fails with the error "Don't know how to build task 'redmine:plugins:assets'"ClosedMarius BĂLTEANU

Actions
Related to Redmine - Patch #40221: Update wiki content related to how to create a custom themeNew

Actions
Related to Redmine - Patch #40559: Fix incorrect icon image paths for Wiki help pagesClosedMarius BĂLTEANU

Actions
Actions #1

Updated by Dmitry Lisichkin 6 months ago

Hello. Sorry for long response.
Am I right that your idea is about using propshaft without other tools (without compilation)?
In my work I start from another side - compilation with old good sprockets as digest generator.

Actions #2

Updated by Dmitry Lisichkin 6 months ago

And after all I can say that your way can be much simplier because of using tools like webpack or other requires many changes in existed js files of redmine.
For example application.js is just a big helper that in propper way should be splitted to many small libraries.
Another problem is a huge usage of js.erb files that directly call functions from application.js and other js files.

Actions #3

Updated by Dmitry Lisichkin 6 months ago

One other problem that we have on our installation - many existed themes and plugins assumed that redmine assets available on root of public directory (not in assets subdirectory), I see a lot of hardcoded pathes in js and css that use absolute and relative paths to the root of redmine hosts.
As result I still have old mechanic with copying assets to root of public directory for backward compability.
Good alternative is usage of old behaviour for plugin_name/assets/* and use new behaviour for some other directories like plugin_name/app/assets/*.

Actions #4

Updated by Marius BĂLTEANU 4 months ago

  • Target version set to 6.0.0

Let’s discuss this for Redmine 6.0.0.

Actions #5

Updated by Marius BĂLTEANU 4 months ago

Takashi Kato, this is a very nice improvement, thank you for your hard working on this.

I made some quick tests now and I didn't find any major problem, I was able to use the current trunk version of Redmine with all three patches applied and two plugins:

Also, I've quickly created a fourth patch (attached) that removes the manually hardcoded rails-ujs library from jquery-3.6.1-ui-1.13.2-ujs-7.1.2.js and uses the one provided by the actionview gem, it's working very well.

Some notes:
1. Task bin/rails redmine:plugins:assets no longer works:

bin/rails aborted!
NoMethodError: undefined method `mirror_assets' for Redmine::PluginLoader:Class

        Redmine::PluginLoader.mirror_assets(name)
                             ^^^^^^^^^^^^^^
/work/lib/tasks/redmine.rake:164:in `block (3 levels) in <top (required)>'

Should we adapt it to generate the assets for a specific plugin or completely drop it.

2. For Redmine core assets, as next improvement after integrating these patches, I would like to move our existing assets to app/assets for the sake of consistency with Rails default structure and to not provide in the public folder two folders, the "source" assets and the actual assets (generated with Asset Pipeline). I'm aware that more changes are required, but I do not see a real problem to include all of them in 6.0.0.

3. Dmitry Lisichkin has a point in #note-3, but at the same time, I think the complexity to keep both behaviours for plugins do not worth it. I'm even in favour to move the plugin assets in "app/assets" to force plugins to use the correct way and also to stick to the Rails structure. The changes to adapt a plugin to this breaking change are minimal.

Actions #7

Updated by Takashi Kato 4 months ago

Marius BĂLTEANU
Thank you for your review and positive feedback.

I agree with matching the behavior of the plugin to Railsway as much as possible.
I've updated the 0001-patch to fix the bug that prevents reloading when some assets are modified in development mode(the name of instance variable was incorrect).

Dmitry Lisichkin
You're correct to point out that if an asset is hard-coded in JavaScript under the 'public' directory, that section of the JavaScript requires modification when applying this patch. However, such path definitions can also cause issues like incorrect cache retrieval during version upgrades.
Modifying the image retrieval method in line with the Asset Pipeline introduction could address this.

"js.erb" files in the app/views directory are returned by the controller and not handled with the asset pipeline.
While Ruby on Rails has recently seen the role of "js.erb" replaced by Hotwire's Turbo Streams, that's distinct from the changes under discussion here.

Actions #8

Updated by Marius BĂLTEANU 3 months ago

  • Assignee set to Marius BĂLTEANU

I've committed the first 4 patches in order to enable the asset pipeline. I'm aware that it's a big and potentially breaking change so I wanted to integrate this as soon as possible in order to have enough time to get feedback from everyone involved.

I'm going to make more changes in the following days, my goal is to match as much as possible the rails recommended directory structure with minimal changes for plugins developers.

Actions #9

Updated by Marius BĂLTEANU 3 months ago

Takashi Kato, do you have any clue why the following test fails on Redmine CI, but not on local environment?

Failure:
Redmine::AssetPathTest#test_asset_path_size [test/unit/lib/redmine/asset_path_test.rb:31]:
Expected: 2
  Actual: 3

bin/rails test test/unit/lib/redmine/asset_path_test.rb:30
Actions #10

Updated by Dmitry Lisichkin 3 months ago

Marius BĂLTEANU wrote in #note-9:

Takashi Kato, do you have any clue why the following test fails on Redmine CI, but not on local environment?

[...]

Can it be .svn directory?

Actions #11

Updated by Marius BĂLTEANU 3 months ago

Dmitry Lisichkin wrote in #note-10:

Marius BĂLTEANU wrote in #note-9:

Takashi Kato, do you have any clue why the following test fails on Redmine CI, but not on local environment?

[...]

Can it be .svn directory?

It could be, I've added some test code to see what asset_path contains, now we need to wait for the CI to run.

Actions #12

Updated by Marius BĂLTEANU 3 months ago

Fix the failing test in r22641.

Dmitry Lisichkin, thanks for your advise!

Actions #13

Updated by Takashi Kato 3 months ago

Marius BĂLTEANU
Thank you for the detailed review and merge!!

Dmitry Lisichkin
I didn't realize it! Thanks!

Actions #14

Updated by Marius BĂLTEANU 3 months ago

  • Subject changed from Asset Pipeline Integration in Redmine for Rails 7 without modifying existing themes to Enable Asset Pipeline Integration
Actions #15

Updated by Marius BĂLTEANU 3 months ago

  • Related to Patch #40137: Jstoolbar help files should import images from the asset pipeline added
Actions #16

Updated by Marius BĂLTEANU 3 months ago

  • Subject changed from Enable Asset Pipeline Integration to Enable Asset Pipeline Integration using Propshaft

I changed all CSS files from public directory to use absolute url to images which is Propshaft expected format.

Takashi Kato, you're implementation Redmine::Asset.new is very useful and I think we should keep it for at least 1 major version in order to give plugins developer time to update their files, but with a deprecation message in the logs. What do you think?

Also, I've added a new issue that we need to fix #40137.

Actions #17

Updated by Marius BĂLTEANU 3 months ago

  • Related to Defect #40204: `rake redmine:plugins` fails with the error "Don't know how to build task 'redmine:plugins:assets'" added
Actions #18

Updated by Marius BĂLTEANU 3 months ago

I made the following changes:

I'm wondering if it's a good ideea to split our assets from the one owned by outside entities, such as jQuery, jQuery UI or Tribute and move all these to vendor/javascripts and vendor/stylesheets.

Actions #19

Updated by Mizuki ISHIKAWA 3 months ago

Marius BĂLTEANU wrote in #note-18:

I made the following changes:

I'm wondering if it's a good ideea to split our assets from the one owned by outside entities, such as jQuery, jQuery UI or Tribute and move all these to vendor/javascripts and vendor/stylesheets.

I've been using a non-standard theme( e.g., https://github.com/farend/redmine_theme_farend_basic , https://github.com/farend/redmine_theme_farend_bleuclair ) with Redmine, and after the changes in r22696, I needed to do the following to keep the theme working:

1. Move the theme from public/themes to app/assets/themes.
2. Change the path where the theme imports Redmine's application.css. (Refer to the commit: https://github.com/farend/redmine_theme_farend_bleuclair/commit/c06330c064d19b02ea3bb6ca7e9a5c37d6c33519)

However, making the change in step 2 causes the theme to break in earlier versions of Redmine, so I'm looking for a way to make the change compatible with both versions.
Additionally, when I added the standard theme to app/assets/themes, it became a target for git staging. I would appreciate a change in .gitignore to prevent this.

diff --git a/.gitignore b/.gitignore
index f25b049a5..f38d49c39 100644
--- a/.gitignore
+++ b/.gitignore
@@ -27,10 +27,10 @@
 /public/assets/*
 /public/dispatch.*
 /public/plugin_assets/*
-/public/themes/*
-!/public/themes/alternate
-!/public/themes/classic
-!/public/themes/README
+/app/assets/themes/*
+!/app/assets/themes/alternate
+!/app/assets/themes/classic
+!/app/assets/themes/README
 /tmp/*
 /tmp/cache/*
 /tmp/pdf/*

Actions #20

Updated by Marius BĂLTEANU 3 months ago

Mizuki ISHIKAWA wrote in #note-19:

Marius BĂLTEANU wrote in #note-18:

[...]

I've been using a non-standard theme( e.g., https://github.com/farend/redmine_theme_farend_basic , https://github.com/farend/redmine_theme_farend_bleuclair ) with Redmine, and after the changes in r22696, I needed to do the following to keep the theme working:

1. Move the theme from public/themes to app/assets/themes.
2. Change the path where the theme imports Redmine's application.css. (Refer to the commit: https://github.com/farend/redmine_theme_farend_bleuclair/commit/c06330c064d19b02ea3bb6ca7e9a5c37d6c33519)

Yes, both changes are required. The first change could be avoided if we keep the themes folder in public/themes, but I find very confusing to have the sources and the assets in the same public directory.

However, making the change in step 2 causes the theme to break in earlier versions of Redmine, so I'm looking for a way to make the change compatible with both versions.

Indeed, I don't see right now a way to keep them compatible, but I will investigate more in the following days.

Additionally, when I added the standard theme to app/assets/themes, it became a target for git staging. I would appreciate a change in .gitignore to prevent this.
[...]

I will update the ignore files for git, mercurial and svn, thanks!

Regarding the directory for non-standard themes, what do you think if we move them to themes in the root directory as we have for plugins? The structure will be the following:

app/assets/themes
  +- alternate/
  +- classic/

themes/
  +- <theme name>/
       |
       +- favicon/
       |    +- <favicon file> (e.g. favicon.ico, favicon.png)
       |
       +- javascripts/
       |    +- theme.js
       |
       +- stylesheets/
            +- application.css

In this way, when you install Redmine and you want to customize it, you work only with themes and plugins directories, you don't need to browse the internal structure from app.

Actions #21

Updated by Marius BĂLTEANU 3 months ago

  • Related to Patch #40221: Update wiki content related to how to create a custom theme added
Actions #22

Updated by Mizuki ISHIKAWA 2 months ago

Marius BĂLTEANU wrote in #note-20:

Regarding the directory for non-standard themes, what do you think if we move them to themes in the root directory as we have for plugins? The structure will be the following:
[...]

In this way, when you install Redmine and you want to customize it, you work only with themes and plugins directories, you don't need to browse the internal structure from app.

I agree. Having a similar structure to the plugins directory ensures consistency and should make it easier to understand for both new users and existing users.

Actions #23

Updated by Marius BĂLTEANU 2 months ago

  • Category set to Code cleanup/refactoring
  • Status changed from New to Resolved
  • Resolution set to Fixed

Now that #40137 is finished, I think we can consider this feature done from developing point of view. I will keep this ticket as Resolved until we prepare a detailed changelog and a wiki page.

Please open new issue if you find any problem!

Actions #24

Updated by Marius BĂLTEANU 11 days ago

  • Related to Patch #40559: Fix incorrect icon image paths for Wiki help pages added
Actions #25

Updated by Marius BĂLTEANU 11 days ago

  • Status changed from Resolved to Closed

I think we can consider this feature done, please open new tickets in case of issues.

Thanks again to everyone who contributed to this change.

Actions

Also available in: Atom PDF