[BUG] js asset added inline doesn't respect order

jimblue commented 6 years ago

Hi there,

It seems that when adding a JS file inline trough param of Grav assets manager, the order is not respected. Inlined assets are always rendered on the last position.

e.g. this:

{% block javascripts %}
  {% do assets.addJs('theme://js/manifest.js, {'priority':100, 'loading':'inline'}) %}
  {% do assets.addJs('theme://js/main.js', {'priority':10}) %}
{% endblock %}
{{ assets.js }}

render into:

<script src="/user/themes/mytheme/js/main.js" type="text/javascript" defer=""></script>
<script>inlined content of manifest.js file</script>

It's a problem that js rendering order doesn't respect the given order in Grav assets manager params as it can break the js code.

Cheers

rhukster commented 6 years ago

As discussed in slack, this is really a significant change, and i've marked it as an enhancement because it will require a fair bit of work that has backwards compatibility implications. I'll look at it when i get a chance.

jimblue commented 6 years ago

Thank you 😄

hughbris commented 3 years ago

I've just encountered this error and set up a test for reporting it, then found this. I think it's important the order is respected. I don't think it's an enhancement, rather a bug.

(I guess I can work around this using small assets groups or moving my inline assets to files. I'm making a rough interim port of a terrible WP site, so it's probably a poor setup to start with.)

Since I've gone to the trouble, here are my test steps in case you need them:

Create a stub page at 99.test/assets.md ..

---
title: Test
never_cache_twig: true
---
Here's some content.

Create some asset files in the theme's js directory to output to console. Here's js/test/file1.js, repeat and modify for file2.js and file3.js.

console.log('file 1');

Create a template templates/assets.html.twig in the theme ..

<html>
<head>
<title>Assets test only</title>

{% set inline_js1 %}
console.log('inline 1');
{% endset %}

{% set inline_js2 = "console.log('inline 2');" %}

{% block javascripts %}
    {% do assets.addInlineJs(inline_js1, 100) %}
    {% do assets.addJs('theme://js/test/file1.js', 90) %}
    {% do assets.addJs('theme://js/test/file2.js', 80) %}
    {% do assets.addInlineJs(inline_js2, 70) %}
    {% do assets.addJs('theme://js/test/file3.js', 60) %}
{% endblock %}

{% block assets deferred %}
    {{ assets.css()|raw }}
    {{ assets.js()|raw }}
{% endblock %}

</head>

<body>
{% block content %}
    {{ page.content|raw }}
{% endblock %}
</body>

</html>

My console output was:

file 1
file 2
file 3
inline 1
inline 2

expected:

inline 1
file 1
file 2
inline 2
file 3

Some notes about the template:

I have also tried setting priority with verbose arguments (not shorthand) and also removing priority, which should respect the order
originally my theme folder was called js/tests (with an 's') and for some reason this serves 403 (with content text/plain) as reported in #2224 (but only the assets in that folder). Simply renaming the folder to anything else fixes this (so not permissions). Is there something magic about the name "tests"? I'll report a separate issue if you see one.

hughbris commented 2 years ago

I just encountered this again, searched issues, and realised what a poor memory I have! I'm surprised this is still unresolved. Has this slipped off the radar? I'm curious if there are still backwards compatibility implications and what they are if so.

rhukster commented 2 years ago

I think it kinda has slipped off the radar. I had forgotten about it.. It's kind of an edge case, so not commonly encountered. I will try to take a look at it soon but am swamped with work at the mo.

hughbris commented 2 years ago

Thanks for the honest response. I'll argue that this is a bug and not an enhancement because:

there is a priority parameter for assets
inline and resource assets can be mixed
the expectation is that these can be mixed and priority will be respected within an asset group - the docs don't say otherwise and it seems like a natural expectation.

I also don't agree it's an edge case because I've been tripped up by it twice now in six months (for all of ten head-scratching minutes, lol). When I port themes (as in both these cases), I don't like to second guess why assets are included in the order they are provided, or deal with any possible consequences from changing them. Sure, a lot of times it won't turn out to matter, but I'd rather assume it does. I presume this bug also affects CSS assets, where order within a group is much more likely to matter.

So please consider relabeling this as a bug. I have an OK workaround and will try not to get fooled again, so it's not keeping me awake. It's just a chink in Grav's "works as it says on the tin" armour IMO, that I'd prefer to see fixed.

getgrav / grav

[BUG] js asset added inline doesn't respect order #1893