Loris icon indicating copy to clipboard operation
Loris copied to clipboard

Refactor `NDB_Page::getCSS/JSDependencies()`

Open maximemulder opened this issue 1 year ago • 2 comments

Brief summary of changes

While browsing the Loris code, I noticed that the methods NDB_Page::getCSSDependencies() and NDB_Page::getJSDependencies() are defined in a way that forces each redefinition to duplicate (more than 50 times) the appending of the dependencies and the prepending of the Loris base URL to each dependency. This is bad for several reasons : duplication (duplicated code), complexity (repeated use of parent / array_merge), verbosity (because of the duplicated code), fragility (forgetting to duplicate the logic leads to a bug).

I propose this simplified model that divides getCSS/JSDependencies() method in two:

  1. NDB_Page::getCSS/JSDependencies(), redefined in each final class that returns the page-specific dependencies of that class.
  2. NDB_Page::getAllCSS/JSDependencies(), defined only in NDB_Page, that calls the previous method, appends the returned page-specific dependencies to the global ones, and prepends the Loris base URL to each dependency.

If I take a random file, here is the result:

Before

    function getJSDependencies()
    {
        $factory = \NDB_Factory::singleton();
        $baseURL = $factory->settings()->getBaseURL();
        $deps    = parent::getJSDependencies();
        return array_merge(
            $deps,
            [
                $baseURL . "/statistics/js/table_statistics.js",
                $baseURL . "/statistics/js/form_stats_MRI.js",
            ]
        );
    }

After

    function getJSDependencies(): array
    {
        return [
            '/statistics/js/table_statistics.js',
            '/statistics/js/form_stats_MRI.js',
        ];
    }

Further thoughts

  • I personally prefer to put the method CSS before the JS one, as it is arguably more simple and generally appears first in a web page.
  • All the dependencies used $baseURL, thus enabling its factoring. If there is ever a need for an external dependency, it is possible to create getExternalDependencies() methods. I think segregating internal and external dependencies is a good thing.
  • This new model does not support "intermediate" dependencies (defined in neither the base nor the final class). I think this is a good thing, as deep inheritance hierarchies should generally be avoided. The only case it appeared before this PR was in NDB_Menu_Filter, which however only includes library React components (tables...) that should already be imported by each module that uses them.
  • Having to manually rewrite the PHPDoc for each redefined method seems suboptimal to me. I think it would be better to simmply use @inheritDoc, which the linter does not seem to detect right now. I'll also add that many previous comments were outdated (listing files that were no longer there) or inconsistent with one another (different formats for the same meaning).

Testing instructions

  1. Browse Loris and verify the CSS/JS files are correctly included.

Links to related issues

  • PR #9073 fixes some missing imports that were exposed by this PR.

maximemulder avatar Feb 19 '24 21:02 maximemulder

I believe the tests will pass once the linked PR is merged.

EDIT: It did 🥳

maximemulder avatar Feb 20 '24 14:02 maximemulder

As per the last meeting, I added @ridz1208 as a reviewer. Just tell me what you think of this idea of a refacor. If you like it, I'll rebase it, add methods for external dependencies (getExternalCSS/JSDependencies), and we'll talk further about it in another meeting or merge.

maximemulder avatar Jul 25 '24 14:07 maximemulder