-
-
Notifications
You must be signed in to change notification settings - Fork 274
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
feat: HTML/CSS/JS ContextWriter #636
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress on this. Thanks for working on it.
I think we need to keep the text length exactly the same, so that we can apply changes made by the LSP.
However, some of these changes adjust the size of content. I think it would be worth adding an assertion for each test that:
- The number of lines in the output is the same as the input.
- That each line is the same length as the input.
That could be modelled by:
func getLineLengths(s string) (lengths []int) {
lines := strings.Split(s, "\n")
lengths = make([]int, len(lines))
for i := 0; i < len(lines); i ++ {
lengths[i] = len(lines[i])
}
return lengths
}
Then, compare the arrays with cmp.Diff
or something.
input: "background-color:\r\n{ constants.BackgroundColor };\r\n", | ||
name: "css: single constant property with windows newlines", | ||
input: "background-color:\r\n{ constants.BackgroundColor };\r\n", | ||
expectedCSS: "background-color: ' ';\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For target LSPs to work well, the character positions can't change in the file.
Our goal is to be able to send the current file position (line, col) to the LSP and have it send text insertions and other things back to templ. When we receive those changes, ideally, I'd want to be able to put them into the file without having to remap their positions.
However, this change removes the \r\n
and \r
chars, therefore changing the line pos.
Here, we're losing \r\n
, so the line position
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected CSS is presumably the output after parsing, which is essentially the same as what we get if we've formatted the code.
However, in our case, we want to support the user continuously typing into the editor, before they've formatted the code, so I'm not sure this is the right behaviour. Maybe it is, and I'm not understanding the context of the test though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected CSS is presumably the output after parsing, which is essentially the same as what we get if we've formatted the code.
I believe this to be the behavior that is causing all of these length mismatches. Since the entire document is parsed into a series of nodes and then built back up with the formatting specified in all of the Write()
methods, this is the default output after the changes I've made with the ContextWriter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the suggested line length checking to all of the unit tests that I updated. There were quite a few mismatches outside of the ones you caught so those are failing as of 4c7959b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started on a solution in another branch on my fork: alehechka/templ@feature/html-lsp...alehechka:templ:fix/mismatch-line-lengths.
The rough idea is to include a perservePositions
bool on the ContextWriter that will determine how certain nodes get formatted. As of this comment, this is resolving the manipulation of self-closing tags. The next step is reconciling extraneous white space between the source and output.
@@ -180,6 +220,9 @@ func TestCSSParser(t *testing.T) { | |||
input: `css Name() { | |||
background-color: #ffffff; | |||
}`, | |||
expectedCSS: ` | |||
background-color: #ffffff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, here, we've got a bit of extra spacing.
@@ -210,6 +253,9 @@ background-color: #ffffff; | |||
input: `css Name() { | |||
background-color: { constants.BackgroundColor }; | |||
}`, | |||
expectedCSS: ` | |||
background-color: ' '; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too.
@@ -256,6 +302,9 @@ background-color: { constants.BackgroundColor }; | |||
input: `css Name(prop string) { | |||
background-color: { prop }; | |||
}`, | |||
expectedCSS: ` | |||
background-color: ' '; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More additional space.
input: `<a href="test"/>`, | ||
name: "element: self-closing with single constant attribute", | ||
input: `<a href="test"/>`, | ||
expectedHTML: `<a href="test"></a>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, since this changes the position of text in the file, I don't see how this could support the goal of allowing text insertion easily.
input: `<a href={ "test" }/>`, | ||
name: "element: self-closing with single expression attribute", | ||
input: `<a href={ "test" }/>`, | ||
expectedHTML: `<a href=" "></a>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also changes in size.
input: `<hr/>`, | ||
name: "element: self-closing with no attributes", | ||
input: `<hr/>`, | ||
expectedHTML: `<hr/>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, on void elements, there's a discussion on supporting them in the templ parser. Basically, I think it might be a good idea because some users expect them to be supported and get confused, see: #637 (comment)
I think the plan might be to "fix" <hr>
and make it into <hr/>
(just for void elements).
input: `<hr style="padding: 10px" />`, | ||
name: "element: self-closing with attribute", | ||
input: `<hr style="padding: 10px" />`, | ||
expectedHTML: `<hr style="padding: 10px"/>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the other position changes.
expectedHTML: `<hr | ||
style="padding: 10px" | ||
|
||
class="itIsTrue" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the HTML LSPs will do about duplicate attributes, but I guess we can deal with that later.
@a-h, thanks for the feedback. For the ones that changed, it seemed that the writer was enforcing certain HTML rules. The main one is changing self-closing tags to having an actual closing tag. This does show up when the document is formatted so I chose to leave the rewrites as-is and adjust the expected output in the unit tests. Do you have a recommendation on how this should be handled for passing to the HTML LSP? Since this is built with the ContextWriter on top of the existing |
Alternative at the moment on vscode. Switch Language servers between templ and html. templ for go code and html for html, css, js completion |
Overview
This is another step toward HTML LSP support in templ, related to the discussion in #498. This PR specifically implements the ContextWriter that @a-h proposed in this comment.
With these changes, I updated nearly all the unit tests to include an expected output format for each context where appropriate (i.e. JS for JS, HTML for HTML). We could add expected output for each individual context, but I skipped on that for now since they would all end up being a huge series of whitespace characters.