Theme Check not checking ParserBlockingJavaScript

Shopify / theme-check-action

Run shopify/theme-check on GitHub pull requests

Other

38 stars 14 forks source link

Theme Check not checking ParserBlockingJavaScript #47

Closed binarymonkey84 closed 2 months ago

binarymonkey84 commented 2 months ago

I ran a test action on a pull request, where I'd added a parser blocking script tag.

Theme Check did not add any line annotations next to the code, even though the docs say this check is on by default and severity is error.

Theme Check reference for this specific issue: https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/parser-blocking-javascript

Screenshot of commit in PR with annotations elsewhere from Theme Check, but not on the script tag where it's expected:

Is this action somehow out of date with the latest checks, or is there some bug or configuration issue?

I did notice on the "Theme Check Report" under "ThemeCheck Configuration" that ParserBlockingJavaScript is not listed, but ParserBlockingScript is. Not sure if that's the issue or not?

binarymonkey84 commented 2 months ago

Just to update - I had a little look to see how the action gets a list of checks, which is here: https://github.com/Shopify/theme-check-action/blob/41d1a848e28b1232d8f2252a24a34d8c7f30faa4/src/steps/getConfigContents.ts

When I run the command shopify theme check --output=json --print in my terminal with CLI version 3.64.1, the output contains the ParserBlockingScript check name, so I guess that's coming from the CLI tool itself.

And in any case, I noticed that the CLI tool doesn't pick up on the check regardless of what it's called.

Perhaps this issue should be moved to https://github.com/Shopify/theme-tools/issues?

karreiro commented 2 months ago

👋 Hey @binarymonkey84,

Thank you for bringing this up!

Currently, Theme Check doesn't support static analysis for parsing block scripts like this:

<!-- Bad -->
<script>
  $('#thing').click(() => {
    alert('Hello!');
  });
</script>

<!-- Good -->
<script>
  document.addEventListener('DOMContentLoaded', () => {
    $('#thing').click(() => {
      alert('Hello!');
    });
  });
</script>

However, Theme Check does support scenarios like these:

<!-- Bad -->
<script src="https://code.jquery.com/jquery-3.6.0.min.js"></script>

<!-- Good -->
<script src="https://code.jquery.com/jquery-3.6.0.min.js" defer></script>

Given the complexity and impact involved in the first scenario, I unfortunately don't anticipate that feature being introduced in Theme Check.

Thank you for bringing this up tho!

binarymonkey84 commented 2 months ago

@karreiro Thanks for clarifying, although the documentation describes the check as follows:

"Identifies HTML script tags that don't have defer or async attributes."

The documentation should be updated to say that this check does not include inline script tags, and also should use the correct check token that's used by Theme Check - ParserBlockingScript and not ParserBlockingJavaScript.

Wouldn't it still be beneficial for Theme Check to at least raising a warning level for parser blocking inline script tags? I'm not suggesting Theme Check analyse the code within a script tag, just check for the attribute in the same way it's already doing for externally loaded script tags. Parser blocking inline script tags are still a performance issue, and it would be better to use defer attributes on those where possible.

Thanks

charlespwd commented 2 months ago

Parser blocking inline script tags are still a performance issue.

:wave: I find myself a bit less worried about those than I am about downloading random libs from CDNs which incur quite a lot more costs: download(https handshake, server latency, download), decompress, parse, execute. An inline script tag is already available, is generally smaller and only has the parse + execute cost.

It's still parse + execute... but, I don't know, doesn't feel like quite the footgun in terms of scale vs the download + decompress in the <head>. Inline script tags are also usually down the documents, images and CSS has probably already loaded...

Do you have a particular common use case in mind that you think is particularly bad?

binarymonkey84 commented 2 months ago

Should Theme Check recommend inline script tags be deferred?

With performance, every little counts - you can easily end up with a problem caused by "death by a thousand papercuts" (lots of individually small problems, added up to cause a problem). The performance cost of inline script is relative to the volume and syntax used. It could easily be the case that small blocks of inline code could have a fairly hefty toll on performance.

Because of this, I consider it best practice to defer when possible, whether it's externally loaded or inline. Of course, sometimes we actually need it not to be deferred, and that's probably more likely with inline script. Hence why I suggested a warning level not error, but Theme Check's existing mechanism to ignore checks for blocks of code also helps here.

Is the documentation accurate?

To my other point, the docs seem factually incorrect to me. They state script tags are checked, but there's nothing to say that's not checking inline script and only externally loaded scripts. At the least we should update the docs, unless we're extending Theme Check to issue warnings on non deferred inline script.

Hope that clarifies! Let me know what you think.

karreiro commented 2 months ago

Thank you for sharing those points, @charlespwd and @binarymonkey84!

While script tags with the src attribute have indeed the largest performance impact, I agree with @binarymonkey84 that the documentation could be clearer. Thus, I've created an internal issue to address fix this.

However, that doesn’t mean you can’t have a check in your project, @binarymonkey84. Here’s how to create your own check: https://github.com/Shopify/theme-tools/tree/main/docs/theme-check-common/writing-your-own-check

To implement that check, you can reuse the existing ParserBlockingScript code and remove this condition to achieve that :)

Thanks again for bringing this up!