grav icon indicating copy to clipboard operation
grav copied to clipboard

Published 20 hours ago • verified publisher icon getgrav

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

Open jimblue opened this issue 6 years ago • 6 comments

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

jimblue avatar Feb 27 '18 17:02 jimblue

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.

rhukster avatar Mar 01 '18 21:03 rhukster

Thank you 😄

jimblue avatar Mar 01 '18 21:03 jimblue

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 avatar Aug 22 '21 23:08 hughbris

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.

hughbris avatar Dec 09 '21 10:12 hughbris

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.

rhukster avatar Dec 09 '21 18:12 rhukster

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.

hughbris avatar Dec 13 '21 08:12 hughbris