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

Refactor NDB_Page::getCSS/JSDependencies() #9074

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MaximeMulder
Copy link
Contributor

@MaximeMulder MaximeMulder commented Feb 19, 2024

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

@MaximeMulder MaximeMulder marked this pull request as ready for review February 19, 2024 21:59
@MaximeMulder
Copy link
Contributor Author

MaximeMulder commented Feb 20, 2024

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

EDIT: It did 🥳

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.

None yet

1 participant