Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validates whole partial #350

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

titoBouzout
Copy link
Contributor

@titoBouzout titoBouzout commented Sep 15, 2024

This is a working proof of concept on validating the whole partial, that would need a few tweaks and real world testing.

Some context, tried to run a headless chrome instance to actually validate the partials with a real browser. The differences between es6/require and idk what else made me give up. Also, Im not so convinced that headless chrome will run on all the enviroments this is actually used, and could cause some problems. So been thinking the behaviour of innerHTML must be specified, and possibly jsdom (an already know dependency) could maybe do the work. After some tests, I suspect thats the case.

Looking at an actual issue solidjs/solid#2279

running the following in chrome

Document.parseHTMLUnsafe(`
<a>
  <div>
    <span><i></i></span>
    <span></span>
  </div>
  <div>
    <hr>
    <div>
      <a></a>
      <a>
`).body.innerHTML

yields

<a>
  <div>
    <span><i></i></span>
    <span></span>
  </div>
</a>
<div>
  <a><hr /></a>
  <div>
      <a></a>
      <a></a> 
      <a></a>
  </div>
</div>

running the same through jsdom

<a>
  <div>
    <span><i></i></span>
    <span></span>
  </div>
</a>
<div>
  <a><hr /></a>
  <div>
    <a></a>
    <a></a>
    <a></a>
  </div>
</div>

So I think it's safe to assume that jsdom may be used for validating complete partials, however, I'm not confident about how this will behave in the wild, it will need real world testing.

Considerations:

  • main idea is to validate the nesting of markup, for this reason, html comments(possible to try to include them in the future if the approach works, because these are relevant to some rendering stuff), and text content is removed from the validation.
  • the throw path.buildCodeFrameError it's showing an unrelated path, Im not sure if thats because of the test environment. I tried to show the relevant path but Im not that confident of the best location, so left it there for people that are more familiar.
  • had to remove all the closing tags for the comparation, it would be better probably if we can have the string of the template with the closing tags, maybe as template.templateWithClosingTags, so the comparation would be more faithful, and also at the same time we could be checking that omitting closing tags won't bring unknown problems.
  • Im not sure if the location for the validation is the best possibly there's a better location for validating?
  • the current validation of checking for parent->child, its actually nice, so we dont have to run this code in case of an error
  • would be nice to provide a diff with the error message

Remaining validation related stuff that I could remember:

@titoBouzout
Copy link
Contributor Author

titoBouzout commented Sep 15, 2024

Improvements:

  • dom-expressions comments are now also validated
  • template.templateWithClosingTags will accumulate template.template but using the closing tags, so the validation is more faithful, the error message easier to understand and will help to locate where the issue is.
  • added diffs to the error message

The error diff It's supposed to be something like this, without the grey text I suppose

WindowsTerminal_UtUh7iqhdF

There's an informative message on top of the diff that reads like

The HTML provided is malformed and will yield unexpected output when evaluated by a browser.

@titoBouzout
Copy link
Contributor Author

titoBouzout commented Sep 16, 2024

I have added this validation to my transform, running it against my full docs site found two things to handle (now fixed):

  • text content outside tags wasn't handled, somehow I have a partial that its just text with a random <mark> tag in the middle, the difference between doesn't vs doesn&#39;t was messing it up.
  • tables, this one was kind of expected, a partial <tr>..</tr> will yield by a browser <table><tbody><tr>..</tr></tbody></table>. I think it will be safe to assume the user will use the <tr>..</tr> inside a table/tbody, so we can special case the validation by adding the missing bits (so that's what I did on the previous commit). I can't think of another tag that behaves like this. In any case I guess we can also special case it.

@titoBouzout
Copy link
Contributor Author

titoBouzout commented Sep 16, 2024

Improvement:

  • came to my mind that instead of removing the text nodes we could replace them by the string #text(or anything else), that way we will detect if the browser "moves" text nodes of location, handling the issue "text node cannot be child of TR"

Testing the code from the linked issue:

Document.parseHTMLUnsafe(`
<table><thead><tr>#text</tr></thead></table>
`).body.innerHTML

yields

#text<table><thead><tr></tr></thead></table>

This is now handled by the previous commit

@titoBouzout
Copy link
Contributor Author

titoBouzout commented Sep 19, 2024

The following thread has a weird use case of trying to do something ilegal.
https://discord.com/channels/722131463138705510/1286323202871525456

They are trying to stop the event from bubbling to the other form.

<form>
  <Portal>
    <form>
      <button type="submit" /> submit events   
    </form>
  </Portal>
</form>

Nesting forms is disallowed, you can get nested forms by using the form attribute on the input elements.
https://playground.solidjs.com/anonymous/a61c7d21-5eb9-4996-93b5-3deb0c64aefd

Anyway, what caught my attention about this, is that it will create 2 partials, and skip our validation. A possible solution, is to traverse the component ahead of time and accumulate the supposed JSX tags, to do something similar to template.template +=.. but without separating the component in a new partial. This way we can get validation working at least at a component level. It will be some progress over no validation at all, and separates the validation from the transformation. It won't catch mixing identifiers into the JSX, nor validating what Portal returns, but still could be an improvement.

uh, thinking a bit more, this example that came to mind will raise a validation error. But maybe.... on this case we can skip tables/tr/td validation, as these elements are the exception. Something to think

<div>
  <TableSomething>
    <td>
      <button type="submit" /> submit events   
    </td>
  </TableSomething>
</div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant