feat: add new option `html-top-level-indent` #6077

Justineo · 2019-04-23T06:12:44Z

Added a new option: html-top-level-indent, which is described in #3888 (comment).

The new option has the following possible values:

"always" - Always apply top-level indent for templates, scripts and styles.
"never" - Avoid top-level indent for templates, scripts and styles.
"auto" - Avoid top-level indent for scripts and styles inside Vue files.

I’ve added tests to confirm my change works.
(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
(If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
I’ve read the contributing guidelines.

lydell · 2019-04-23T07:30:38Z

I like it! 👍

Considering the last couple of comments in #3888 I'm also open to only doing --vue-indent true/false (should be an easy change). What to think, @azz (re #5574 (review))?

Justineo · 2019-04-23T08:15:47Z

@lydell Should I update the website code as well? (Not sure how to do it and may need some help.)

Actually, one of my concern is whether we should apply this to all embedded language block inside HTML, or just to the top-level.

The current implementation will possibly produce the following markup (with htmlTopLevelIndent: false):

<script>
let foo
</script>
<body>
  <script>
    let bar
  </script>
</body>

lydell · 2019-04-23T16:16:02Z

I'd say we should apply it to all levels:

<script>
let foo
</script>
<body>
  <script>
  let bar
  </script>
</body>

But this feels like another argument to making the option specific to Vue SFCs.

Should I update the website code as well?

Do you mean the playground? I haven't touch it since the React rewrite, so I'm a bit unsure myself.

ForsakenHarmony · 2019-04-23T20:08:09Z

To me indenting template but not script and style (in vue) by default seems like an inconsistency, should at least not be the default

Justineo · 2019-04-24T02:17:26Z

@ForsakenHarmony Changing the default behavior will be a breaking change.

ForsakenHarmony · 2019-04-24T15:36:02Z

I know, but maybe it should be

azz · 2019-04-25T08:48:42Z

I'm not sure I like the name for auto, since it's vue-specific it should something like not-vue, no?

Justineo · 2019-04-25T09:17:04Z

auto is for those who do not care about this option. We shouldn’t expose implementation details in the option value to those users IMO. It’s not Vue-specific and will apply to any embedded language inside an HTML context.

tszulc · 2019-05-08T21:06:14Z

Is there any word on when this will go in?

lydell · 2019-05-09T05:45:59Z

The code of this PR looks really good! I tried it out locally and realized something.

The reason I thought #5574 (review) sounded like a good idea was because it sounded like there would be nothing Vue-specific about the option. But there is.

~/forks/prettier implement-3888 %  
❯ ./bin/prettier.js test.vue --html-top-level-indent auto
<template>
  <div>test</div>
</template>

<script>
test;
</script>

~/forks/prettier implement-3888 %  
❯ ./bin/prettier.js test.vue --html-top-level-indent always
<template>
  <div>test</div>
</template>

<script>
  test;
</script>

~/forks/prettier implement-3888 %  
❯ ./bin/prettier.js test.vue --html-top-level-indent never
<template>
<div>test</div>
</template>

<script>
test;
</script>

Notice the indentation of <template> with the different option values. This does work as described by the help text and PR description, but didn't realize this until now.

Now I'm a little confused. What is it that users want really?

It makes sense from a logical point of view not to indent <template> if you're not indenting <script> and <style>. But nobody has complained about that? It feels more like there are two camps in #3888: Those who want to keep things as-is, and those who want to indent everything.

So maybe a --vue-indent-script-and-style boolean flag would make more sense? (On the other hand, we could just as well end up in the situation with --jsx-bracket-same-line where people want it in HTML as well: #5377)

Sorry for all the back and forth! I really wanted to press the "Approve" button on this PR this morning, but now I'm not sure. Prettier's goal is a low amount of options. When we add a new one we must make sure that it is really making people happier. And I'm not sure that auto users will be happy with never (if we remove auto in 2.0) since that also dedents <template>. Or maybe they would? Who knows? 😢

bytesnz · 2019-05-09T07:56:01Z

My understanding is the auto value will keep the indentation as it currently is with the HTML and vue formatters (the indenting template but not script and style seems to be one of the standards in vue) and the always and never values will change to be all indented or not indented. The PR works as I was hoping it would.

Justineo · 2019-05-09T08:38:38Z

@lydell

Sorry for all the back and forth! I really wanted to press the "Approve" button on this PR this morning, but now I'm not sure.

No worries.

What I had in my mind when making this PR is:

provide a language-agnostic option API for all markup languages
keep current behavior as default to prevent a breaking change
having always but no never seems a little weird to me

But as you say, there's not yet huge demand for never so I'm totally okay with dropping never (though I personally prefer never but auto is just fine as well).

So maybe a --vue-indent-script-and-style boolean flag would make more sense? (On the other hand, we could just as well end up in the situation with --jsx-bracket-same-line where people want it in HTML as well: #5377)

I see #5377 has a lot of upvotes, and I think it doesn't make much sense that we only allow it in JSX but not HTML (I'd say people waiting for that feature can't be very happy about it). So in this case I agree with your earliest opinion (#3888 (comment)) to make it work beyond Vue templates.

And I'm not sure that auto users will be happy with never (if we remove auto in 2.0) since that also dedents <template>.

If you are dropping the current behavior, I'd say it will be very likely that some users will get unhappy, whether you have never or not. I think we should consider whether to apply the option beyond Vue files first, then talk about possible option values and wording.

ForsakenHarmony · 2019-05-10T01:57:08Z

auto to me seems like a very misleading name if it only changes something for Vue, I'd really prefer for it to not be merged in the current form.

If not making this very specific thing which most people don't like according to the polls a breaking change is important, the default value should have some kind of special name

Justineo · 2019-05-10T02:14:21Z

@ForsakenHarmony auto actually means you don't have strong opinions on this and just let Prettier handle this by conventions we can find (in this case: the recommendation of the Vue.js official style guide). And since it applies to all HTML-base language, I don't think we should add a Vue-specific option value. Maybe I should change the help text.

lydell · 2019-05-10T05:35:59Z

always – the easy to understand one
auto – the Vue one
never – the one you won't use

🤔

ForsakenHarmony · 2019-05-11T12:21:37Z

@Justineo thing is it currently is Vue specific?

Justineo · 2019-05-11T13:08:01Z

@ForsakenHarmony Yes.

kamilic · 2019-05-26T09:48:17Z

Any plans to merge these changes in next version?

lydell · 2019-05-26T09:53:25Z

I think the conclusion from what we’ve learned from this PR is that the --html-top-level-indent option sounded good on paper, but reality calls for a Vue-specific option --vue-indent-script-and-style.

kamilic · 2019-05-26T10:44:56Z

I vote --vue-indent-script-and-style.
I think that no or few people will use this as html-specific option.
Maybe we can move this feature to internal, providing ability for--vue-indent-script-and-style implementation

Justineo · 2019-05-26T11:18:21Z

As mentiond earlier, this PR aims to:

Answer the needs of Vue/Prettier users.
Respect the opinion of the Prettier team, to make it available for all HTML-base languages: Indent script and style tags content in *.vue files #3888 (comment), Indent script and style tags content in *.vue files #3888 (comment). And as @lydell said it's more future-proof and help us avoid situations like [resolved] [HTML] allow corresponding option to jsxBracketSameLine #5377.
Avoid introducing breaking changes.

Unfortunately, it's not ideal in the end. Let's wait for a vue-indent-script-and-style implementation then.

lydell · 2019-05-26T12:16:31Z

@Justineo Thank you for your hard work on this PR! I’m really sorry it did not end up merged, but sometimes a PR of exploration is what it takes to find the best solution to a problem. 🙇‍♂️

Justineo · 2019-05-26T12:26:03Z

To be clear, I don't think vue-indent-script-and-style is a better solution. 😕

But I respect your choice.

…r#6077

ForsakenHarmony · 2019-05-26T21:24:35Z

I like this, but I just don't like auto as the name for the current behaviour

…r#6077

* feat: implement --vue-indent-script-and-style mentioned at pr-#6077 * docs: --vue-indent-script-and-style * update new test case * feat: playground for --vue-indent-script-and-style * chores: Revert package.json version * Remove noisy snapshots

…ttier#6157) * feat: implement --vue-indent-script-and-style mentioned at pr-prettier#6077 * docs: --vue-indent-script-and-style * update new test case * feat: playground for --vue-indent-script-and-style * chores: Revert package.json version * Remove noisy snapshots

Justineo added 2 commits April 23, 2019 14:10

feat: add html-top-level-indent option, resolve prettier#3888

9d5c682

docs: update docs for html-top-level-indent

029e91d

Justineo mentioned this pull request Apr 23, 2019

Indent script and style tags content in *.vue files #3888

Closed

feat: apply top-level indent to any nested && embedded language

c282dc7

Justineo force-pushed the implement-3888 branch from f7cf274 to c282dc7 Compare April 25, 2019 01:41

Justineo closed this May 26, 2019

Justineo deleted the implement-3888 branch May 26, 2019 11:18

kamilic pushed a commit to kamilic/prettier that referenced this pull request May 26, 2019

feat: implement --vue-indent-script-and-style mentioned at pr-prettie…

eac4c3d

…r#6077

kamilic mentioned this pull request May 26, 2019

feat: implement --vue-indent-script-and-style mentioned at pr-#6077 #6157

Merged

4 tasks

kamilic pushed a commit to kamilic/prettier that referenced this pull request Jul 18, 2019

feat: implement --vue-indent-script-and-style mentioned at pr-prettie…

46bde72

…r#6077

kamilic pushed a commit to kamilic/prettier that referenced this pull request Aug 1, 2019

feat: implement --vue-indent-script-and-style mentioned at pr-prettie…

9f59b21

…r#6077

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 24, 2019

lock bot locked as resolved and limited conversation to collaborators Aug 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new option `html-top-level-indent` #6077

feat: add new option `html-top-level-indent` #6077

Justineo commented Apr 23, 2019 •

edited

lydell commented Apr 23, 2019

Justineo commented Apr 23, 2019 •

edited

lydell commented Apr 23, 2019

ForsakenHarmony commented Apr 23, 2019

Justineo commented Apr 24, 2019

ForsakenHarmony commented Apr 24, 2019

azz commented Apr 25, 2019

Justineo commented Apr 25, 2019

tszulc commented May 8, 2019

lydell commented May 9, 2019 •

edited

bytesnz commented May 9, 2019

Justineo commented May 9, 2019 •

edited

ForsakenHarmony commented May 10, 2019

Justineo commented May 10, 2019 •

edited

lydell commented May 10, 2019

ForsakenHarmony commented May 11, 2019

Justineo commented May 11, 2019

kamilic commented May 26, 2019

lydell commented May 26, 2019

kamilic commented May 26, 2019 •

edited

Justineo commented May 26, 2019 •

edited

lydell commented May 26, 2019

Justineo commented May 26, 2019

ForsakenHarmony commented May 26, 2019

feat: add new option html-top-level-indent #6077

feat: add new option html-top-level-indent #6077

Conversation

Justineo commented Apr 23, 2019 • edited

lydell commented Apr 23, 2019

Justineo commented Apr 23, 2019 • edited

lydell commented Apr 23, 2019

ForsakenHarmony commented Apr 23, 2019

Justineo commented Apr 24, 2019

ForsakenHarmony commented Apr 24, 2019

azz commented Apr 25, 2019

Justineo commented Apr 25, 2019

tszulc commented May 8, 2019

lydell commented May 9, 2019 • edited

bytesnz commented May 9, 2019

Justineo commented May 9, 2019 • edited

ForsakenHarmony commented May 10, 2019

Justineo commented May 10, 2019 • edited

lydell commented May 10, 2019

ForsakenHarmony commented May 11, 2019

Justineo commented May 11, 2019

kamilic commented May 26, 2019

lydell commented May 26, 2019

kamilic commented May 26, 2019 • edited

Justineo commented May 26, 2019 • edited

lydell commented May 26, 2019

Justineo commented May 26, 2019

ForsakenHarmony commented May 26, 2019

feat: add new option `html-top-level-indent` #6077

feat: add new option `html-top-level-indent` #6077

Justineo commented Apr 23, 2019 •

edited

Justineo commented Apr 23, 2019 •

edited

lydell commented May 9, 2019 •

edited

Justineo commented May 9, 2019 •

edited

Justineo commented May 10, 2019 •

edited

kamilic commented May 26, 2019 •

edited

Justineo commented May 26, 2019 •

edited