Skip to content

Support single-file component inputs #29

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

Merged
merged 7 commits into from
Jun 5, 2025
Merged

Support single-file component inputs #29

merged 7 commits into from
Jun 5, 2025

Conversation

lucaswerkmeister
Copy link
Member

The “meat” of this pull request is in the last commit, the rest are minor cleanups and preparatory refactorings.

Bug: T395802

Otherwise, IntelliJ complains that this is missing.
Component’s responsibilities now start at the DOMElement; HtmlParser
contains the code to parse the HTML and extract the root node from the
DOMDocument. (I want to extend the latter logic in a follow-up commit,
and for this it’s nicer if it’s a separate class, IMHO.)

In principle, Component::render() should probably operate on a deep
clone of the root node; then a Component could be reused without having
to parse it several times, which could come in handy later. However, at
the moment, the Component is always freshly created, so the extra clone
seems wasteful; so for now just document that render() shouldn’t be
called repeatedly. The only “public” entry point to this library so far
is Templating, which obeys this rule.

Also add covers tags to TemplatingTest, since it’s kind of an
integration test for much of the library (including JsParsing, but I
can’t be bothered to add all those classes to the covers as well).

Bug: T395802
Previously, we’d crash because the documentElement is null in this case.
So we can also use it in Component::appendHTML() (v-html="" handling).
Part of this will also be useful when getRootNode() gains support for
SFC inputs later (which is why appendHTML() doesn’t use getRootNode()
directly).

In theory, we could skip over a <head> element in the <html>, but as I
don’t think it makes sense to have a <head> in any Vue component /
template, let’s just throw an error in that case and include a test for
that. (I don’t think we can test the “expected <html>” condition, that
part is basically dead code – the parser should always synthesize <html>
and <body> elements surrounding “normal” markup if it’s not already
present in the source.)
This is not only more readable, but also a bit faster [1] (though I
doubt this code is a bottleneck anywhere either way).

[1]: https://3v4l.org/NGEKe
If the input contains a single root <template> and the other root
elements are all <script> or <style>, then pick the <template> contents
as the actual root element and only render its contents. This allows us
to use SFC *.vue files as the input.

It’s worth noting that we can’t use getBodyElement() directly, as I had
originally hoped – if the SFC starts with a <script> and then has a
<template> afterwards, rather than starting with the <template>, then
the PHP HTML parser will actually place the whole contents inside a
synthetic <head> rather than a synthetic <body>. (Such an SFC structure
is disallowed by eslint-config-wikimedia via vue/component-tags-order,
but I don’t think this library should rely on that.)

Bug: T395802
@codders
Copy link
Contributor

codders commented Jun 5, 2025

These changes look good. I guess we'll need to update the integration tests at some point so that they can also handle SPCs and that we can add some more examples there. But we can also do that in the coming changes as we add more functionality.

@codders codders self-requested a review June 5, 2025 11:02
Copy link
Contributor

@codders codders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - thanks for the changes!

@codders codders merged commit 158a0ff into master Jun 5, 2025
10 checks passed
@codders codders deleted the mex branch June 5, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants