Move local JS files to the header using defer

Created on 19 June 2024, 6 months ago
Updated 12 July 2024, 5 months ago

Problem/Motivation

Posting as a bug report because it seems the intent was there, but the execution wasn't accordingly. Basically, the two local JS files seem to want to be put in the header, but instead of specifying header: true, head: true was set. Then, only the 2nd local JS file seems to be using defer, potentially indicating this was the intent for both of them.

Steps to reproduce

View cookies.library.yml, notice the incorrect "head" property and fact that only one out of two local JS files has the defer attribute.

Proposed resolution

Put both files in the header using defer.

Remaining tasks

Clarify original intent, review MR, accept or reject MR

User interface changes

/

API changes

/

Data model changes

/

🐛 Bug report
Status

Fixed

Version

1.2

Component

Code

Created by

🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @kristiaanvandeneynde
  • Status changed to Needs review 6 months ago
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
  • Pipeline finished with Success
    6 months ago
    Total: 367s
    #202872
  • 🇩🇪Germany Anybody Porta Westfalica

    Thanks @kristiaanvandeneynde!

    head: true is definitely wrong!

    For the other changes: I guess this makes a lot of sense. I'm not yet sure if this can break anything in the COOKiES functionality or cause race-conditions. But thinking about this for a while I don't think so, because the script-blocking is server side.

    Any other potential risks you can see?

  • 🇩🇪Germany Anybody Porta Westfalica

    As the preloader already had the defer attribute, I agree it might make sense to have the same for the conf.

    After testing this, we should first keep it in dev and test it for some time. The most risk I can see is, that other scripts and especially analytics scripts execution might change? (But I actually don't think so, just to write down my thoughts and potential side-effects).

    Another thing we need to ensure is, that the both libraries are loaded in the correct order.

    @kristiaanvandeneynde I'd very much like to have your feedback and also ping @Grevil and @JFeltkamp for feedback here, before we commit this.

  • Assigned to Grevil
  • 🇩🇪Germany Anybody Porta Westfalica
  • Issue was unassigned.
  • Status changed to RTBC 6 months ago
  • 🇩🇪Germany Grevil

    LGTM! Just tested a bit around with it. Everything is still working as intended!

  • Status changed to Fixed 6 months ago
  • 🇩🇪Germany Grevil

    Merged!

    Didn't change the MR title, so it doesn't show up in this issue.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Hey, sorry for being unresponsive. I'm currently at Dev Days and not keeping an eye on my issue queue as much. Thanks for committing this, though!

  • 🇩🇪Germany Anybody Porta Westfalica

    No worries @kristiaanvandeneynde! :) But please feel free to test the latest dev and reply here.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024