fix: Improve post-build asset update check

[Snugug]

Description

This currently fails on empty attributes (#4030) and links to MPA output (href="/my/mpa/output") that doesn't have a direct disk counterpart. Checking for these things resolves these errors.

Resolves #4030

Additional context

This fixes the MPA issue I'm experiencing (the above link would error as trying to read a directory, related to #4030 but a slightly different usecase). This should resolve all of them.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[Snugug]

I didn't see any tests for the HTML plugin so I didn't add any for this functionality; hopefully that isn't a blocker to get this to land.

[patak-dev]

The html playground is testing the html related plugins

[patak-dev]

LGTM, while we wait for an extra approval it would be good if you could add a simple check in the html playground to avoid future regressions

[Snugug]

Ok! I've added a couple of test files; the empty attributes one fails w/o this patch, the link one seems to pass with both, but it'll fail in my codebase w/o my code, so there must be some edgecase that this resolves that isn't getting triggered where absolute anchor links get triggered as attributes and throw this error. That said, I also notice that the HTML playground isn't rendering the rollup input as I would expect per the MPA docs, so maybe that's why it's not being triggered.

[haoqunjiang]

The empty attribute case should fail with a better error message, not silently pass.

In what scenario would you need a <img src="">?

[patak-dev]

Let's hold releasing while we discuss, sorry if I merged too quickly. I would prefer a warning here myself instead of a hard error, so people can still preview their work, but it is true that this change should be better evaluated first

[Snugug]

For that specific question, there's a common lazy loading pattern that uses this, and when using templating systems it's possible to end up with empty attributes. In general, though, I think the bigger thing is that it's throwing for otherwise valid HTML and you don't get any sort of warning or error in dev for it, only build. It's also only throwing because it's trying to replace the URL with whatever the final output URL is (which, for an empty attribute, is still empty)

…

On Dec 15, 2021, at 6:16 AM, Haoqun Jiang ***@***.***> wrote:  The empty attribute case should fail with a better error message, not silently pass. In what scenario would you need a <img src="">? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[haoqunjiang]

For that specific question, there's a common lazy loading pattern that uses this

Can you elaborate?

[Snugug]

It's basically this article but with an src="" to ensure the img tag is valid.

The tl;dr of that article is: image tag, no src (or for this, empty src so it's a valid image), use IntersectionObserver to figure out when the image is visible, then swap in the data-src to the src

[haoqunjiang]

Got it. Thanks for the explanation.

I previously thought that <img src=""> would initiate a request to the document itself. Turns out that's not the case.
An empty URL referenced in CSS (e.g. background-image: url("")) would do so, though.

As this PR only touches the code dealing with HTML asset URLs, I think the change is indeed an improvement.
Thanks!

[Snugug]

no problem, and thank you!

…

On Dec 16, 2021, at 1:52 AM, Haoqun Jiang ***@***.***> wrote:  Got it. Thanks for the explanation. I previously thought that <img src=""> would initiate a request to the document itself. Turns out that's not the case. An empty URL referenced in CSS (e.g. background-image: url("")) would do so, though. As this PR only touches the code dealing with HTML asset URLs, I think the change is indeed an improvement. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.