Feature: check empty string reference for other resource in HTML(and SVG) #7318

Shinyaigeek · 2021-11-17T16:34:27Z

↪️ Pull Request

Hi team 👋

I investigated about #7076, and I found this is caused by an empty string uniqueKey in Asset. uniqueKey’s default value is an empty string “”, and this and asset with empty string specifier breaks dependency graph at https://github.com/parcel-bundler/parcel/blob/v2/packages/core/core/src/AssetGraph.js#L517-L519. This is the cause of the strange error.

Of course, If I set a string other than an empty string to uniqueKey, the dependency graph will be built as expected, but this empty string reference will be resolved as / path and causes another strange behavior.

After consideration, I fixed this issue via checking whether or not a reference value in HTML (and also SVG) is empty string in Transforming as SassTransformer checks whether the imported file exists.

If there is some context I missed or you have some opinion about this approach, I will be happy if you point it out or feel free to close this PR.

💻 Examples

<html><body><img src="" /></body><script src="" type="module"></script></html>

🚨 Test instructions

I made a test case in html in integration tests to check parcel correctly throw an error or not with html like an above example.

✔️ PR Todo

Added/updated unit tests for this change
Filled out test instructions (In case there aren't any unit tests)
Included links to related issues/PRs

…g html and svg

…rce in html

height · 2021-11-17T16:34:29Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

packages/transformers/html/src/dependencies.js

devongovett · 2021-11-23T02:36:21Z

Should we error on this, or should we just ignore empty strings? We would currently ignore if the attribute is not there entirely. Maybe someone has a reason for doing this?

Co-authored-by: Devon Govett <devongovett@gmail.com>

mischnic · 2021-11-23T09:48:22Z

Browsers don't ignore empty src attributes either:
https://humanwhocodes.com/blog/2009/11/30/empty-image-src-can-destroy-your-site/
https://stackoverflow.com/a/5946696

Shinyaigeek · 2021-11-23T09:48:33Z

According to whatwg, a reference to the other resource such as href in or src in should be non empty string valid URL, so HTMLTransformer (and also SVG) throws an error when handling empty string href. However, I did not know this context( > We would currently ignore if the attribute is not there entirely. ).

I think it is also reasonable to ignore empty string href, just as Parcel ignores the attribute which is not there entirely. On the other hand, I also think that giving an error on an invalid href will lead more kindful Developer Experience, what do you think about this?

FYI: https://html.spec.whatwg.org/multipage/semantics.html#attr-link-href
https://html.spec.whatwg.org/multipage/embedded-content.html#attr-img-src

devongovett

Makes sense! Thanks.

folknor · 2021-11-29T20:03:06Z

This is presenting me warnings like these ones:

@parcel/transformer-html: value should not be empty string

  /.../index.html:968:11
    967 |                   <div class="form-check mb-3">
  > 968 |                     <input class="form-check-input" type="checkbox" value="" id="test1"
  >     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  > 969 |                       checked>
  >     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    970 |                     <label class="form-check-label" for="test1">
    971 |                       ...

@parcel/transformer-html: checked should not be empty string

  /.../index.html:968:11
    967 |                   <div class="form-check mb-3">
  > 968 |                     <input class="form-check-input" type="checkbox" value="" id="test1"
  >     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  > 969 |                       checked>
  >     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    970 |                     <label class="form-check-label" for="test1">
    971 |                       ...

But according to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input/checkbox both these values can be omitted:

If the value attribute was omitted, the default value for the checkbox is on, so the submitted data in that case would be subscribe=on.

and

To make a checkbox checked by default, you give it the checked attribute. See the below example:

(example uses an empty checked attribute)

Shinyaigeek added 2 commits November 18, 2021 01:18

Feature: check empty string refrence to other resource in transformin…

57c5777

…g html and svg

Chore: add test for checking no empty string reference to other resou…

46cb844

…rce in html

Merge branch 'v2' into fix/throw-reference-empty-string

c53eba8

DeMoorJasper approved these changes Nov 21, 2021

View reviewed changes

devongovett reviewed Nov 23, 2021

View reviewed changes

packages/transformers/html/src/dependencies.js Outdated Show resolved Hide resolved

Shinyaigeek and others added 2 commits November 23, 2021 18:40

Update packages/transformers/html/src/dependencies.js

3ae11ea

Co-authored-by: Devon Govett <devongovett@gmail.com>

Merge branch 'v2' into fix/throw-reference-empty-string

3390164

Shinyaigeek added 2 commits November 24, 2021 18:34

Merge branch 'v2' into fix/throw-reference-empty-string

a6c89d8

Merge branch 'v2' into fix/throw-reference-empty-string

4a61c72

devongovett approved these changes Nov 28, 2021

View reviewed changes

devongovett merged commit 5b606ea into parcel-bundler:v2 Nov 28, 2021

mischnic mentioned this pull request Nov 29, 2021

Only error for empty HTML dependency attributes #7383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: check empty string reference for other resource in HTML(and SVG) #7318

Feature: check empty string reference for other resource in HTML(and SVG) #7318

Shinyaigeek commented Nov 17, 2021 •

edited

height bot commented Nov 17, 2021

devongovett commented Nov 23, 2021

mischnic commented Nov 23, 2021

Shinyaigeek commented Nov 23, 2021 •

edited

devongovett left a comment

folknor commented Nov 29, 2021

Feature: check empty string reference for other resource in HTML(and SVG) #7318

Feature: check empty string reference for other resource in HTML(and SVG) #7318

Conversation

Shinyaigeek commented Nov 17, 2021 • edited

↪️ Pull Request

💻 Examples

🚨 Test instructions

✔️ PR Todo

height bot commented Nov 17, 2021

devongovett commented Nov 23, 2021

mischnic commented Nov 23, 2021

Shinyaigeek commented Nov 23, 2021 • edited

devongovett left a comment

Choose a reason for hiding this comment

folknor commented Nov 29, 2021

Shinyaigeek commented Nov 17, 2021 •

edited

Shinyaigeek commented Nov 23, 2021 •

edited