Based on the discussion in #3301609: JS has hard dependency of cookies/cookiesjsr causes double load of cookiesjsr-preloader I think we should remove the additional cookies/cookiesjsr.cdn library and instead use cookies/cookiesjsr as only library definition for cookiesjsr.
With a hook_library_info_alter we should change the source, if CDN is enabled.
Why?
At first glance this sounds a bit weird, but I think it's an antipattern to use different libraries for the same "library" depending on the source. As library dependencies are kind of "API" which other modules can depend on, they would otherwise have to duplicate the logic for CDN / local switch.
Anyway this is minor, as typically COOKiES submodules and extending modules don't have to have a dependency on cookiesjsr if they call none of the cookiesjsr API functions. Only reacting to an event doesn't require a dependency in code. Nothing breaks, if the event isn't called!
That also turned out in the linked issue.
hook_library_info_alter to change the source of cookies/cookiesjsr for CDNcookies/cookiesjsr.cdnNone
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | Selection_999(661).png | 142.7 KB | gilmord |
| #9 | 2022-08-09 14_29_54-Window.png | 6.3 KB | grevil |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
anybodyComment #5
grevil commentedUnclear what exactly this means. My approach would be like this:
Is the cookiesjsr library installed? -> Do nothing
Is the cookiesjsr library not installed? -> Use the cdn library
And to implement this, I would overwrite the library attributes via "hook_library_info_alter()".
Comment #6
grevil commentedAh, ok the cdn option is a configuration in the cookies base settings. Then I will use @Anybody's approach.
Comment #7
anybody@Grevil: As just talked... I'll give a short & dirty example in the MR as starting point.
Comment #8
anybodyThis is just an example for the general way to go... please check where
cookiesjsr.cdnis used / included etc. - hope things got clearer now when looking at the code and reading the issue summary again? :)Comment #9
grevil commentedOk, the cdn option works as expected, but I have no idea, why downloading the library does not properly work...
I will get the following error:
Obviously, the path is incorrect. Rather than pointing to "dist/cookiesjsr-preloader.min.js/cookiesjsr.min.js" it should point to "dist/cookiesjsr-preloader.min.js", but I can't find the code, where this behaviour gets overwritten. If I change the library definition from "/libraries/cookiesjsr/dist/cookiesjsr-preloader.min.js" to "/libraries/cookiesjsr/dist". The error code will point to ".../libraries/cookiesjsr/dist" as expected and the if case in "cookies_library_info_alter" is not entered...
Comment #10
grevil commentedAlso tests fail...
Comment #11
grevil commentedI changed the library string but commented it out for testing and forgot to comment it in again...
Sorry, Tests should work now, but the problem in #9 still stands.
Comment #12
grevil commentedThe problem was a faulty "libPath" annotation!
@JFeltkamp what is the main difference between cookiesjsr-preloader.min.js and cookiesjsr.min.js? Just better performance?
Comment #13
anybodyAs we're not sure if
cookiesjsr-preloader.min.jsorcookiesjsr.min.jsis the right file to use, we should wait für @JFeltkamp's feedback.For now, we've chosen
cookiesjsr-preloader.min.js!We couldn't find documentation for this on the cookiesjsr page and due to the minified source, we can only guess... So would be cool if Joachim could review, comment and perhaps improve the documentation on the Github page? :)
I added an issue at GitHub for this: https://github.com/jfeltkamp/cookiesjsr/issues/20
Thanks a lot in advance and well done @Grevil! :)
Afterwards this is ready for the final review, but already close to RTBC ;) (+1 from my side)
Comment #15
gilmord1. Library is added to the page multiple times (see attached screenshot) with this MR, working on it.
2. Pushed a small commit with the code-style fixes.
Comment #16
anybodyThanks for the review @gilmord!
We should then please add a test to detect the duplicate library first, which fails and succeeds after the fix.
Comment #17
gilmordFound commit with git bisect, here it is:
https://git.drupalcode.org/project/cookies/-/merge_requests/40/diffs?com...
checking why it caused duplicated libraries
Comment #18
gilmordIssue with the preloader, if use cookiesjsr.min.js - all works
if use cookiesjsr-preloader.min.js - duplicated libraries appears
Comment #19
gilmord@Anybody pushed a fix with a couple of small fixes (code styles, dependency injection)
Please create a separate issue to cover this with tests so we do not block the fix
Comment #20
anybodyThanks! :)
No, that is not the way we should go. This isn't a major bugfix and may introduce further issues, so it should be shipped with its tests, even if it takes more time.
And even if it would be a major bugfix, better fix it with test to not break anything again ;)
This is not Wordpress ;D ;) (Just joking, don't hate me Wordpress)
Comment #21
anybodyHow to proceed:
1. Final feedback from @JFeltkamp for #13 would be nice, even though @gilmord found out that cookiesjsr.min.js seems to be correct
2. We need to ensure we have tests for local library and CDN which should be existing in the module and succeed with and without this MR (means: also before this change)
3. We should do a final manual test with CDN and local to ensure it works, also for submodules.
The 4 failing cookies_facebook tests once again seem unrelated to the issue fork testing issue...?
Comment #22
gilmordFaced same 4 testing errors here:
https://www.drupal.org/project/cookies/issues/3303681
I also created MR with empty commit from 1.0.x to 1.0.x with no changes (only project maintainers can skip these steps and run tests on the specific branch from the UI) - same failed tests,
I can assume it is a test issue, not the current MR issue.
Comment #23
anybody@gilmord: Yes, it is: #3250126: Submodule dependencies are ignored if composer.json present in parent module (only in issue fork test runs!)
The 100% same submodule tests work in the project itself, see https://www.drupal.org/pift-ci-job/2448785 or https://www.drupal.org/project/cookies
But in issue forks / MR's they break... Sadly nobody responded on the CI issue so far...
Comment #24
anybodyThis can be proceeded as of #21. Anyway we should first create a new stable release with the old code and afterwards merge this.
Comment #25
anybodyComment #26
anybody@gilmord, please see https://git.drupalcode.org/project/cookies/-/blob/1.1.x/cookies.librarie...
/libraries/cookiesjsr/dist/cookiesjsr-preloader.min.jswas also used before, not/libraries/cookiesjsr/dist/cookiesjsr.min.js... This makes me wonder about your results from #18. Any explanation?Comment #27
jfeltkamp@Anybody Yes, you are right. It is an antipattern, we should use a single library and update with hook_library_info_alter.
Answere for #12 @Grevil: Yes, the reason is performance. It seems important to me to optimize the performance in this way, because the loading of JS files are chained and COOKiES JS is a bottle neck. The preloader has a size of 33KB what is 10% against the main library, and fires the event to load other knocked-out JS libraries much earlier. And if the cookiesjsr-cookie is set, there is no need to load the main library with the COOKiES UI.
BUT: When the support for IE can be removed, the COOKiES main library will be much smaller (due to all the polyfills). Then we could think about to remove the preloader also. Could be an improvement of jfeltkamp/cookiesjsr Version 2.0.
Comment #28
grevil commented@JFeltkamp, thanks for the clarification!
Comment #29
anybody@JFeltkamp: Is there already an issue for a 2.0 version and the removal of IE support in cookiesjsr?
I think it's time with Drupal 10:
https://www.drupal.org/docs/system-requirements/browser-requirements
Also see:
https://workspaceupdates.googleblog.com/2020/12/ending-support-for-ie11-...
So would it perhaps make sense to target a 2.x release for COOKiES and cookiesjsr with Drupal >=10 support?
Or should we merge this before and try to recreate the libraries used in the libraries.yml file before 1:1? With 2.x that could be dropped then.
Anyway it would be nice if @JFeltkamp could help to review and test this to not break things...
Comment #30
anybodyNew features go into 2.x.
Comment #31
anybodyFixed by @jfeltkamp in 2.x - please help testing 2.0.0-alpha1!!