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

Vue: improve parsing custom blocks with angular-html-parser@1.5.0 #8153

Merged
merged 29 commits into from May 14, 2020

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Apr 25, 2020

Fixes #8108
Fixes #6503
Fixes #6325

Close #7975

<!-- Input -->
<custom lang="javascript">
const foo =    "</";
</custom>

<!-- Prettier stable -->
SyntaxError: Unexpected character """ (2:19)
[error]   1 | <custom lang="javascript">
[error] > 2 | const foo =    "</";
[error]     |                   ^
[error]   3 | </custom>

<!-- Prettier master -->
<custom lang="javascript">
const foo = "</";
</custom>
  • I’ve added tests to confirm my change works.
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@fisker
Copy link
Member

fisker commented Apr 25, 2020

I think we support this before. stable

@sosukesuzuki sosukesuzuki marked this pull request as ready for review April 25, 2020 07:27
@@ -288,6 +339,11 @@ module.exports = {
vue: createParser({
recognizeSelfClosing: true,
isTagNameCaseSensitive: true,
getTagContentType: (tagname) => {
if (tagname !== "template") {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (tagname !== "template") {
if (tagname.toLowerCase() !== "template") {

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this is necessary. I haven't seen template tag written with upper case. I also can't find any such mention in the specification or in the source code of the sfc-compiler.

@sosukesuzuki
Copy link
Member Author

@fisker

I think we support this before. stable

Does it need to be supported by the vue parser? It is supported by the HTML parser(Playground), I think which is sufficient. (We don't have a vue parser tests for it.)

@fisker
Copy link
Member

fisker commented Apr 25, 2020

I don't know, but people can use --vue to parse .html to support v-if.

@thorn0
Copy link
Member

thorn0 commented Apr 26, 2020

I'm not a Vue user, but the name of "single file components" implies that there are "multiple file components" that have their templates in separate files. I skimmed through the Vue docs, found some mentions of "in-DOM templates", but there doesn't seem to be a separate page about this.

@sosukesuzuki
Copy link
Member Author

I forgot about directives. I cannot find about vue app written in only one html file in documentations, but the below code works fine. It would be good that Prettier support this. I'll fix.

<html>
  <body>
    <div id="example">
      <p v-if="hello === 'Hello World!'">{{ hello }}</p>
    </div>
    <script src="https://unpkg.com/vue"></script>
    <script>
        new Vue({
            el: '#example',
            data: { hello: 'Hello World!' }
        })
    </script>
  </body>
</html>

@@ -340,7 +340,7 @@ module.exports = {
recognizeSelfClosing: true,
isTagNameCaseSensitive: true,
getTagContentType: (tagname) => {
if (tagname !== "template") {
if (tagname !== "template" && tagname !== "html") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about template, but I'm sure about html, it can be uppercase :)

Copy link
Member Author

Choose a reason for hiding this comment

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

stable prettier does not support it...(Playground). We should open new issue.

parser=vue

Input:

<!DOCTYPE html><HTML>
  <body>
    <div v-if="foo ===    'foo'">

</div>
    <script>
new Vue({el: '#app'})
    </script>
  </body>
</HTML>

Output:

<!DOCTYPE html
>><HTML>
  <body>
    <div v-if="foo ===    'foo'">

</div>
    <script>
new Vue({el: '#app'})
    </script>
  </body>
</HTML>

Copy link
Member

Choose a reason for hiding this comment

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

We should.

Copy link
Member

Choose a reason for hiding this comment

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

I don't use angular, angular does not support it too

Playground

Copy link
Member Author

Choose a reason for hiding this comment

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

@fisker
Copy link
Member

fisker commented Apr 28, 2020

@sosukesuzuki NOOOO

@sosukesuzuki
Copy link
Member Author

ooops, I'll fix..

@fisker

This comment has been minimized.

@fisker

This comment has been minimized.

@sosukesuzuki

This comment has been minimized.

@fisker

This comment has been minimized.

@fisker
Copy link
Member

fisker commented Apr 28, 2020

@sosukesuzuki see prettier/angular-html-parser#11 (comment)

@fisker
Copy link
Member

fisker commented May 13, 2020

I saw the ci running, then I test those two links. thought it's my problem. The commits are missing on your fork too https://github.com/sosukesuzuki/prettier/commits/8106

@thorn0
Copy link
Member

thorn0 commented May 13, 2020

I successfully fetched them from the fork. Looks like GitHub's UI is out of sync.

@fisker
Copy link
Member

fisker commented May 13, 2020

Screenshot_2020-05-13-23-18-21-899_com chrome canary

@fisker
Copy link
Member

fisker commented May 13, 2020

All issues addressed?

@sosukesuzuki
Copy link
Member Author

Yes all issues were probably fixed. Can we merge this? Should I fix commits history?
(I've pushed these commits from the new machine so maybe something was wrong with my Git config. I'll look into this.)

@fisker
Copy link
Member

fisker commented May 13, 2020

I'm good.

@fisker
Copy link
Member

fisker commented May 13, 2020

I didn't bring this case here #8162 (comment), but you should know.

I try to throw SyntaxError in #8280 #8281, but this PR keeps it as it is , I think it's better.

@thorn0
Copy link
Member

thorn0 commented May 13, 2020

I reported this situation to GitHub. The commits still aren't visible on the UI. Not sure what happens if we do a merge.

BTW, a question on SO relevant to this PR: https://stackoverflow.com/q/61742084/76173

@fisker
Copy link
Member

fisker commented May 13, 2020

I use this script(and textarea) template before, for browsers don't support template. I don't think we should format it as html , if it can format as html why not put in a hidden <div>?

@fisker
Copy link
Member

fisker commented May 13, 2020

Try close and reopen ?

@@ -65,6 +141,7 @@ function ngHtmlParser(
} else if (node instanceof Text) {
node.type = "text";
} else {
console.log(node);
Copy link
Member Author

Choose a reason for hiding this comment

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

oops, I'll remove this

Copy link
Member

Choose a reason for hiding this comment

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

This done?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

GO!

@thorn0
Copy link
Member

thorn0 commented May 13, 2020

@fisker Prettier formats script type="text/markdown". Why not format script type="text/html"?

@sosukesuzuki
Copy link
Member Author

@fisker

I try to throw SyntaxError in #8280 #8281, but this PR keeps it as it is , I think it's better.

I think so too but the behavior is happened by stable Prettier. So we can fix this in other PR.

@sosukesuzuki
Copy link
Member Author

I think we should open new issues for #8153 (comment) and #8153 (comment) , should not fix in this PR.

Comment on lines 81 to 95
const getSameLocationNode = (node) => {
const result = getSecondParse();
return result.rootNodes.find((_node) => {
if (!_node || !_node.startSourceSpan) {
return false;
}
if (
node.startSourceSpan.start.offset ===
_node.startSourceSpan.start.offset
) {
return true;
}
return false;
});
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const getSameLocationNode = (node) => {
const result = getSecondParse();
return result.rootNodes.find((_node) => {
if (!_node || !_node.startSourceSpan) {
return false;
}
if (
node.startSourceSpan.start.offset ===
_node.startSourceSpan.start.offset
) {
return true;
}
return false;
});
};
const getSameLocationNode = (node) =>
getSecondParse().rootNodes.find(
({ startSourceSpan }) =>
startSourceSpan &&
startSourceSpan.start.offset === node.startSourceSpan.start.offset
);

node can't be false, right?

Comment on lines 77 to 80
const getSecondParse = () =>
secondParseResult
? secondParseResult
: (secondParseResult = doSecondParse());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const getSecondParse = () =>
secondParseResult
? secondParseResult
: (secondParseResult = doSecondParse());
const getSecondParse = () =>
secondParseResult || (secondParseResult = doSecondParse());

Comment on lines 115 to 118
const replaceNode = getSameLocationNode(node);
if (replaceNode) {
rootNodes[i] = replaceNode;
}
Copy link
Member

@fisker fisker May 14, 2020

Choose a reason for hiding this comment

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

Suggested change
const replaceNode = getSameLocationNode(node);
if (replaceNode) {
rootNodes[i] = replaceNode;
}
rootNodes[i] = getSameLocationNode(node) || rootNodes[i];

tagName !== "html" &&
!hasParent &&
(tagName !== "template" ||
attrs.find((attr) => attr.name === "lang" && attr.value !== "html"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
attrs.find((attr) => attr.name === "lang" && attr.value !== "html"))
attrs.some(({name, value}) => name === "lang" && value !== "html"))

@sosukesuzuki
Copy link
Member Author

@fisker Thank you for your review! I fixed all reviewed points.

@fisker fisker merged commit 531b7ca into prettier:master May 14, 2020
@fisker
Copy link
Member

fisker commented May 14, 2020

Great job!

@sosukesuzuki sosukesuzuki deleted the 8106 branch May 14, 2020 14:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants