Composer package javascript resolving #2287

RLangeUni · 2022-01-24T23:10:05Z

PR 1424 introduced out of the box loading of themes from composer package modules. But language and javascript files have not been considered yet.

This pull request is an addendum to resolve javascript files, which are configured in a related mixin.config.php. The second commit adds two example packages: worldcat tab on detail view and result grouping. The latter should be sufficient too see javascript working (page reload after click on "group hits").

Maybe you see a more elegant way to load scripts from vendor, but actually files are supposed to be configured in a mixin, located in a "js" directory next to this mixin by convention, and rendered directly. If the basic approach is correct, I would also write a test for both new methods "loadConfiguredJavascriptFilesFromMixin" and "findInPackage".

- load javascript configured in mixin

* scripts of both packages should be executed: ** vendor/finc/vufind-results-grouping/res/theme/js/resultGrouping.js ** vendor/finc/worldcat-search-module/res/theme/js/fetch_libraries.js

demiankatz · 2022-01-25T14:20:51Z

Thanks, @RLangeUni. I have a couple of concerns and suggestions:

1.) Have you tested how this behaves if you have a mix of built-in mix-ins that reside directly in your theme directory, along with third-party mix-ins installed via composer? Ideally, mix-ins in the theme directory should be loaded directly as before, whereas mix-ins in the vendor directory need their content loaded in a different way. It's possible I didn't review the code closely enough, but I'm concerned that the theme directory mix-in scenario may not behave correctly -- either being forced to load indirectly, or possibly double-loading.

2.) While I think this approach could work, it does seem inefficient to potentially embed the same Javascript into the HTML of every page. Not a big deal if it's small, but if we had a mix-in with a large dependency, the loss of caching benefits could be a significant problem. (Unless this is already integrating correctly with the asset pipeline -- which perhaps it is; again, I didn't do a really in-depth review). However, if this doesn't already work with the pipeline, we should try to figure out a way to make it work. Happy to brainstorm some ideas there, but I won't waste space here in case the problem is already solved. :-)

…org#2287)

- set js code as source earlier in InjectTemplateListenerFactory - undo changes in ThemeInfo.php and HeadScript

demiankatz

Thanks for the ongoing work on this, @RLangeUni. I think some of my comments from prior review still apply, but I noticed a few things while re-reviewing after your latest commit (see below). I apologize if any of these comments are unhelpful -- it's been a while since I last looked at this, and I haven't fully refreshed my memory about all the context. Just wanted to keep the conversation moving forward!

config/vufind/config.ini

demiankatz · 2022-04-27T14:27:19Z

config/vufind/worldcat.ini

@@ -0,0 +1,3 @@
+[General]


I realize that this file is probably just here as part of a proof-of-concept example, but having files called worldcat.ini and WorldCat.ini in the same folder is going to be confusing, not to mention completely breaking things in environments like Windows that treat filenames as case-insensitive.

Indeed. I was not aware of this. Although we are not using Windows. I will fix that.

I would suggest that the best solution to this problem might be to create a separate PR that moves the [WorldCat] section of config.ini into the [General] section of WorldCat.ini. I think this separation would make more sense and would be more consistent with the way other backend configurations work. We could use the config.ini settings as a fallback for backward compatibility, of course. I've been meaning to do this for a long time but haven't gotten around to it (and if you don't have time, let me know and perhaps I can put it higher on my own list).

themes/bootstrap3/templates/RecordDriver/DefaultRecord/result-list.phtml

RLangeUni · 2022-04-28T07:11:52Z

I assume the build failed, because I haven't merged latest dev into this branch. I will take care on that.

2.) While I think this approach could work, it does seem inefficient to potentially embed the same Javascript into the HTML of every page. Not a big deal if it's small, but if we had a mix-in with a large dependency, the loss of caching benefits could be a significant problem. (Unless this is already integrating correctly with the asset pipeline -- which perhaps it is; again, I didn't do a really in-depth review). However, if this doesn't already work with the pipeline, we should try to figure out a way to make it work.

So far we haven't used that pipeline in finc. The main problem is, that js files in vendor can not be accessed as public. Are pipeline resource files only supposed to be cached (e.g. in data/public) to be later rendered as sources or could this be "magically" transformed as head link too?

demiankatz · 2022-04-28T10:34:35Z

So far we haven't used that pipeline in finc. The main problem is, that js files in vendor can not be accessed as public. Are pipeline resource files only supposed to be cached (e.g. in data/public) to be later rendered as sources or could this be "magically" transformed as head link too?

Part of the pipeline logic is a public cache directory configured in httpd-vufind.conf. When the pipeline combines/minifies files, it puts them in this directory to make them directly web accessible. It seems possible to me that we could have a similar mechanism that takes arbitrary scripts, creates a filename in the cache derived from a hash of the code, and then adds that as a headlink.

* set isPipelineActive to public

RLangeUni · 2022-06-01T11:12:15Z

It seems I accidentelly merged dev into this PR branch. I will close this PR and open a new one. Sorry for the confusion.

RLangeUni · 2022-06-01T11:44:40Z

Close to start clean from scratch and release-8.

demiankatz · 2022-06-01T11:48:41Z

@RLangeUni, the release-8.0 branch is no longer being developed (apart from critical bug fixes). I'd suggest re-targeting this PR against dev instead of closing it and creating a new one.

demiankatz · 2022-06-01T11:50:13Z

@RLangeUni, I think I have fixed this one -- unless you disagree, I think it is best to continue working here so we don't lose our conversations in progress, etc.

demiankatz · 2022-06-01T11:50:34Z

Also, for future reference, note that even if a PR gets really badly messed up, you can always force-push to the same branch to repair it.

RLangeUni · 2022-06-01T11:55:47Z

I also commited .phpunit.result.cache files. But now I wonder why my test "loadConfiguredJavascriptFilesFromMixin" is broken. I guess the path is wrong.

RLangeUni · 2022-06-01T12:01:32Z

Thanks for fixing the merging issue. Meanwhile I wonder whether the whole effort is worth it. Some description of my last commits:

asset pipeline should work, if its active; if not scripts (and later stylesheets) should be rendered as head script
therefore I made isPipelineActive active in ConcatTrait; Is that protected access mandatory? If so, it should be checked within InjectTemplateListenerFactory.php

RLangeUni · 2022-06-01T12:06:12Z

And so far I have not found a good solution how to not add scripts for all pages.

One possibility could be defining a whitelist of routes in mixin.config.php or module.config.php and check for that route in loadConfiguredJavascriptFilesFromMixin.

demiankatz · 2022-06-01T12:09:08Z

Thanks, @RLangeUni! I've deleted the unwanted cache file for you. I don't see a problem with making isPipelineActive public (though that's a backward breaking change that should take place in a major release -- but since we're going to be starting work on 9.0 in the dev branch later this summer, and I don't expect this to be finished in time for 8.1, I think it will all work itself out).

I'm also not sure that adding scripts for all pages is a big problem. I know that @EreMaijala has found that things perform better overall if you push everything into the asset pipeline and allow the browser cache to hold a single bundle of Javascript, rather than making different bundles in different contexts. For users without the pipeline turned on, this might be an annoyance, but if performance is a priority, they should probably just turn on the pipeline. That's not to say that we shouldn't consider a solution to the problem, but if it's very complicated and adds a lot of configuration, maybe it's not worth it.

…er.lock + composer.json of dev and Revert "DONT MERGE: example packages results-grouping and worldcat search and local mixin" * undo changes and imports for testing only This reverts commit cfb9e5c. This reverts commit a9d34d3.

…ixture to recognize templates folder

RLangeUni · 2022-06-01T14:21:49Z

I merged latest dev again in composer_package_javascript_resolving and removed / reverted changes to test example packages worldcat and result-grouping.

demiankatz

@RLangeUni, thanks for cleaning this up and moving it forward. I have a few questions -- primarily an organizational one -- which I have provided below. I'll do some deeper review and testing once we have talked these details through.

module/VuFindTheme/src/VuFindTheme/InjectTemplateListenerFactory.php

module/VuFindTheme/src/VuFindTheme/ThemeInfo.php

* move loading of files from InjectTemplateListenerFactory to Initializer and HeadScript * undo changes in InjectTemplateListenerFactory and ThemeInjectTemplateListenerFactoryTest * make method isPipelineActive in ConcatTrait protected again * add slashes for checking vendor files and change comment in ThemeInfo.php::findInPackage

robertlange81 · 2022-10-13T08:16:33Z

I have to apologize for the delay, but tried to keep the ball rolling again. Method loadConfiguredJavascriptFilesFromMixin has been removed from the TemplateListenerFactory and is logically divided between HeadScript on one side and the Initializer class + later used ConcatTrait with asset pipeline on the other side now. So it should be a little more clearer.

In "findInPackage" there should only be an absolute path as argument, I changed the corresponding doc comment.

Perhaps this kind of file loading could be used for other files than these in vendor too (https://openlibraryfoundation.atlassian.net/browse/VUFIND-1566). That's why I'm still not sure if that check for vendor is even needed anymore.

demiankatz

Thanks for the progress, @RLangeUni, I think this is beginning to approach its final form! Just a few more questions and suggestions...

module/VuFindTheme/src/VuFindTheme/Initializer.php

demiankatz · 2022-10-13T10:53:46Z

module/VuFindTheme/src/VuFindTheme/Initializer.php

+            $headScript = $this->serviceManager->get('ViewHelperManager')
+                    ->get('headScript') ?? $this->serviceManager->get('headScript');
+            foreach ($templatePathStack as $templatePath) {
+                if (file_exists($mixin = $templatePath . '/../mixin.config.php')) {


Is there a reason to parse the mixin configurations here? This feels like work that the ThemeInfo class should be doing, to keep that all encapsulated in the same place. Is there a way to either pull the needed information with an existing ThemeInfo method (like getMergedConfig), or should we add a new one for this purpose?

module/VuFindTheme/src/VuFindTheme/ThemeInfo.php

module/VuFindTheme/src/VuFindTheme/View/Helper/ConcatTrait.php

module/VuFindTheme/tests/fixtures/vendor/example/res/theme/js/package.js

* get mixin config from ThemeInfo::getMergedConfig * add method isAlreadyWebAccessible in ThemeInfo

* use realtive path for testing * convert absolute in relative path in isAlreadyWebAccessible

…esting

…nitializer

demiankatz

Thanks for the progress, @RLangeUni, this continues to get better and better. However, on the latest review, I think there may be some misunderstandings about getMergedConfig, and I may also have found a more appropriate place to hook in your logic (unless I'm misunderstanding/forgetting a key detail).

demiankatz · 2022-10-17T12:13:02Z

module/VuFindTheme/src/VuFindTheme/Initializer.php

+
+            foreach ($templatePathStack as $templatePath) {
+                $merged = $this->tools->getMergedConfig(
+                    $templatePath . '/../mixin.config.php'


getMergedConfig is not intended to accept a filename as a parameter, but rather a configuration key to retrieve -- so, for example, you could call it with 'js' if you want to get the final merged list of Javascript settings from all the themes and mixins, since that's the array key where Javascript files are listed.

I also notice that this code seems to be assuming that JS files will always be listed as filenames, but that overlooks the array format of configuration that's been supported since release 8.0.

On further reflection, is it possible that all of this logic should be moved out of the Initializer and into the SetupThemeResources helper? That's where we actually load the scripts into the appropriate header or footer location, so would that be the best time to pre-process non-web-accessible scripts?

Thanks for your remarks. GetMergedConfig should indeed keep it's expected behavior. Perhaps by setting the vendor theme as mixin of the current theme set.

But do I understand you correctly and we should not use GetMergedConfig AND the Initializer anymore? I thought it would be neccessary to use the asset pipeline. Or did we loose that option after migrating from InjectTemplateListenerFactory already? SetupThemeResources and the ResourceContainer are quite low level and to move "loadConfiguredJavascriptFilesFromMixin" there seems unhandily, although parsing package resources could be happen only once.

I believe it may not be necessary to use getMergedConfig, and to reduce the number of changes to the Initializer. My thought is this: the purpose of the ResourceContainer is to collect information about resources necessary to provide them to the user. The purpose of SetupThemeResources is to configure the resource-related view helpers based on the content of the ResourceContainer. All of the asset pipeline logic is handled internally by the view helpers.

Since ResourceContainer already represents information about resources as arrays, it should be possible to adjust the Initializer code that populates the ResourceContainer to provide enough context (e.g. theme base path as a second parameter to addJs) to include a "full path to file" element in the information array. With this information present, then when SetupThemeResources loads data into the view helpers, it can make appropriate compensation for files that are identified as existing outside of the web-accessible area based on these paths. I don't think you need to create mixin-specific logic to figure this out.

Of course, it's possible that I'm overlooking something and this will not work -- but it seems like it might be worth considering/investigating. Does that make any sense?

Yes, of course, it does. I wasn't quite sure about the details. I will try that. Thank you.

demiankatz · 2022-10-17T12:14:09Z

module/VuFindTheme/tests/unit-tests/src/VuFindTest/ThemeInfoTest.php

+    {
+        $ti = $this->getThemeInfo();
+        $config = $ti->getMergedConfig(
+            'module/VuFindTheme/tests/fixtures/vendor/example/res/theme/mixin.config.php'


As noted above, I don't think getMergedConfig is supposed to work like this, so this test might need revision.

demiankatz · 2022-10-17T12:14:51Z

module/VuFindTheme/tests/unit-tests/src/VuFindTest/ThemeInfoTest.php

+        $result = $ti->isAlreadyWebAccessible(
+            $file
+        );
+        $this->assertEquals(true, $result);


You could assertTrue() to be slightly more concise.

demiankatz · 2022-10-17T12:14:58Z

module/VuFindTheme/tests/unit-tests/src/VuFindTest/ThemeInfoTest.php

+        $result = $ti->isAlreadyWebAccessible(
+            $file
+        );
+        $this->assertEquals(false, $result);


You could assertFalse() here.

demiankatz · 2022-10-17T12:16:08Z

module/VuFindTheme/src/VuFindTheme/ThemeInfo.php

@@ -274,6 +274,12 @@ public function getMergedConfig(string $key = '', bool $flatten = false): array
            $currentTheme = $allThemeInfo[$currentTheme]['extends'];
        }

+        if (empty($merged)) {


As noted elsewhere, I don't think we need/want to change the behavior of getMergedConfig here. Note that getMergedConfig already has in-progress revisions in #2543. We should try to avoid creating conflicts with that PR if we can (and if there are problems here related to merged configs, we should see if that PR solves them).

demiankatz · 2023-02-15T17:38:58Z

@RLangeUni, I just wanted to check and see if you've had a chance to look at my most recent review. I expect that the window for planning the 9.0 release is beginning to close, so if it is important to you to get this functionality included in VuFind 9.0, we should try to work to get this finished up sooner rather than later. :-)

Thanks again for your work on this, and please let me know what I can do to help move things forward!

demiankatz · 2024-06-20T19:35:18Z

@RLangeUni / @robertlange81, is this work still needed? I was reviewing pull requests without milestones and see that this has not moved in almost two years. If we still need this, we should set a milestone so we don't lose track of it!

RLangeUni added 3 commits January 24, 2022 22:17

Refactoring of template dependency resolving

00488a6

- load javascript configured in mixin

DONT MERGE: example packages results-grouping and worldcat search

cfb9e5c

* scripts of both packages should be executed: ** vendor/finc/vufind-results-grouping/res/theme/js/resultGrouping.js ** vendor/finc/worldcat-search-module/res/theme/js/fetch_libraries.js

DONT MERGE: add composer.lock

dcfb750

EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Apr 11, 2022

[ALLI-7450] Add Finnish Museum of Photography to about_finna (vufind-…

4f2178c

…org#2287)

RLangeUni added 2 commits April 27, 2022 08:52

Refactoring of template dependency resolving

2a0e286

- set js code as source earlier in InjectTemplateListenerFactory - undo changes in ThemeInfo.php and HeadScript

DONT MERGE: use local_theme_example and local_mixin_example

a9d34d3

demiankatz reviewed Apr 27, 2022

View reviewed changes

RLangeUni added 3 commits May 16, 2022 09:43

Merge branch 'dev' into composer_package_javascript_resolving

9e26538

integrate asset pipeline when active

b322ab3

* set isPipelineActive to public

add test for composer_package_javascript_resolving

2d05f3c

RLangeUni closed this Jun 1, 2022

demiankatz reopened this Jun 1, 2022

demiankatz changed the base branch from release-8.0 to csl June 1, 2022 11:48

demiankatz changed the base branch from csl to dev June 1, 2022 11:49

Delete .phpunit.result.cache

eb0cd0f

RLangeUni added 3 commits June 1, 2022 14:26

Merge branch 'dev' into composer_package_javascript_resolving

79a4a31

Refactoring of template dependency resolving: add package.phtml for f…

4c429f4

…ixture to recognize templates folder

demiankatz requested changes Jun 2, 2022

View reviewed changes

module/VuFindTheme/src/VuFindTheme/InjectTemplateListenerFactory.php Outdated Show resolved Hide resolved

module/VuFindTheme/src/VuFindTheme/ThemeInfo.php Outdated Show resolved Hide resolved

module/VuFindTheme/src/VuFindTheme/ThemeInfo.php Outdated Show resolved Hide resolved

RLangeUni added 2 commits October 13, 2022 09:15

Merge branch 'dev' into composer_package_javascript_resolving

af15410

demiankatz requested changes Oct 13, 2022

View reviewed changes

RLangeUni added 4 commits October 17, 2022 09:44

Refactoring of template dependency resolving:

e4824e1

* get mixin config from ThemeInfo::getMergedConfig * add method isAlreadyWebAccessible in ThemeInfo

Refactoring of template dependency resolving:

cfe9a52

* use realtive path for testing * convert absolute in relative path in isAlreadyWebAccessible

Refactoring of template dependency resolving: use relative path for t…

bd51cfd

…esting

Refactoring of template dependency resolving: reduce line length in I…

a2c83dc

…nitializer

demiankatz requested changes Oct 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composer package javascript resolving #2287

Composer package javascript resolving #2287

RLangeUni commented Jan 24, 2022

demiankatz commented Jan 25, 2022

demiankatz left a comment

demiankatz Apr 27, 2022

RLangeUni Apr 28, 2022

demiankatz Apr 28, 2022

RLangeUni commented Apr 28, 2022 •

edited

Loading

demiankatz commented Apr 28, 2022 •

edited

Loading

RLangeUni commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

demiankatz commented Jun 1, 2022

demiankatz commented Jun 1, 2022

demiankatz commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

demiankatz commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

demiankatz left a comment

robertlange81 commented Oct 13, 2022

demiankatz left a comment

demiankatz Oct 13, 2022

demiankatz left a comment

demiankatz Oct 17, 2022

RLangeUni Oct 21, 2022

demiankatz Oct 21, 2022

RLangeUni Oct 21, 2022

demiankatz Oct 17, 2022

demiankatz Oct 17, 2022

demiankatz Oct 17, 2022

demiankatz Oct 17, 2022

demiankatz commented Feb 15, 2023

demiankatz commented Jun 20, 2024

Composer package javascript resolving #2287

Are you sure you want to change the base?

Composer package javascript resolving #2287

Conversation

RLangeUni commented Jan 24, 2022

demiankatz commented Jan 25, 2022

demiankatz 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

RLangeUni commented Apr 28, 2022 • edited Loading

demiankatz commented Apr 28, 2022 • edited Loading

RLangeUni commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

demiankatz commented Jun 1, 2022

demiankatz commented Jun 1, 2022

demiankatz commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

demiankatz commented Jun 1, 2022

RLangeUni commented Jun 1, 2022

demiankatz left a comment

Choose a reason for hiding this comment

robertlange81 commented Oct 13, 2022

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz 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

demiankatz commented Feb 15, 2023

demiankatz commented Jun 20, 2024

RLangeUni commented Apr 28, 2022 •

edited

Loading

demiankatz commented Apr 28, 2022 •

edited

Loading