Extract logic from HAML views
We have many haml
views that includes code that shouldn't be there. It's either accessing the database to request data, which should never be done in the template engine, or is doing data transformation in order to have it in the correct form to initialize vue/javascript components.\
This is a big codesmell and has huge impact in the codebase:
- It's hard to track performance if the view is making request to the database as profilers will point view took
X ms
but that in fact was a DB query to blame. - It's bad code overall, hard to test, the view has little opportunity to cache the compiled version, which can lead to bad performance etc.
We have few alternatives depending on the context where the view is used.
We do use partial views as a way to share "component" code, like app/views/shared/issuable/_label_dropdown.html.haml
, and the way it's current implemented reminds me of PHP code.
In https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9983/diffs#note_155690916 I suggested this to be refactored as few Helper
methods, while keeping the layout as is, calling render partial: '..'
by the helper, but keeping the logic inside it. This allows it to be easily tested without instantiating a highly complex controller x view fixture or mocking so much stuff.
I tried to find something that could help us here, and looks like https://github.com/flyerhzm/rails_best_practices gem can pinpoint things that aren't nice. I didn't go far to evaluate it deeply, but just by running it against app/views
I got 1907 Warnings
which seems pretty accurate to the state our haml views are right now.
You can check it yourself by running the following:
gem install rails_best_practices
rails_best_practices -f html app/views
this will generate a rails_best_practices_output.html
report file, which you can list and inspect the results.
The suggestions by the tool may not match 1:1 to what we are doing or the decisions we made. Like the suggestion to move logic to Model
may better be implemented as move to Presenter
which is what we are moving to.
Also we have zero documented best practices for HAML in doc/development/fe_guide
. Not really sure why, but looks like to me that because HAML runs in the backend, while still "frontend" related, may be there is a lack of ownership from both teams, as no one really knows who should own it (I can be totally wrong on that as well