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

prettier for Vue #491

Closed
octref opened this issue Oct 20, 2017 · 29 comments
Closed

prettier for Vue #491

octref opened this issue Oct 20, 2017 · 29 comments

Comments

@octref
Copy link
Member

octref commented Oct 20, 2017

We already have the infra for splitting Vue component into multiple regions, and currently we feed

  • <template>: none
  • <style>: prettier with parser css / scss / less
  • <script>: prettier with parser babylon / typescript

Instead of waiting for prettier's html formatter, https://github.com/reshape/reshape by @jescalan seems to be in better shape already. With two more things, I think I can put it in as the default html formatter for Vetur:

  • printWidth. This is especially important for wrapping html attributes. eg:

    <div attr="foo" attr="foo" attr="foo" attr="foo" attr="foo" attr="foo" attr="foo" attr="foo" >bar</div>

    to

    <div
      attr="foo"
      attr="foo"
      attr="foo"
      attr="foo"
      attr="foo"
      attr="foo"
      attr="foo"
      attr="foo"
    >bar</div>
  • Handling of interpolation in attributes and mustache brackets. Example from Vue doc: https://vuejs.org/v2/guide/class-and-style.html#Object-Syntax

    <div class="static"
      v-bind:class="{ active: isActive, 'text-danger': hasError }">
    </div>

    The class object can be very long and needs to be formatted to multiline.

Another thing we could do is to make a CLI based on prettier + reshape + vue-template-compiler. Basically just using vue-template-compiler instead of Vetur's region splitting.

vue component -> vue-template-compiler
  - template -> reshape
  - style -> prettier
  - script -> prettier
    -> assemble into prettierified vue component

@jescalan Do you have any thoughts? Would you be interested in working together on this?

@jescalan
Copy link

Sounds good! Yeah would be happy to help if I can. This all seems feasible, although it would require a new set of reshape plugins to accomplish. Have you had a chance to check out how reshape plugins are built? Probably starting with a printWidth plugin would be best, that one seems to me to be simplest.

@octref
Copy link
Member Author

octref commented Oct 22, 2017

@jescalan I'm already modifying some plugins :D
Would you be open to adding printWidth to reshape/beautify as an option?

@jescalan
Copy link

@octref Absolutely, I would love that in fact.

@StarpTech
Copy link
Contributor

StarpTech commented Aug 18, 2018

Hi @octref WDYT about https://github.com/StarpTech/prettyhtml it's already capable of parsing Vue templates (including self-closing elements).

@StarpTech
Copy link
Contributor

@octref
Copy link
Member Author

octref commented Aug 19, 2018

@StarpTech Thanks a lot for suggesting prettyhtml! I currently don't have much time to work on it myself so it would be great to integrate your work into Vetur.

However, for a good Vue-html formatter, I think printWidth and handling of the interpolations would be essential. I do see printWidth which is great!

Here are a few feedback / issues:

  1. Remove multiple white-lines: it should reduce n (n >= 1) white-lines to 1 white-lines, but not down to 0.

  2. For this case,

<!--[if lte IE 9]>
    <p class="browserupgrade">You are using an <strong>outdated</strong> browser. Please <a href="https://browsehappy.com/">upgrade your browser</a> to improve your experience and security.</p>
    <![endif]-->

The comment regions start/end identifiers need to have the same indentation. You either apply no transformation to all of them or apply a unified indent/outdent to all its children.

  1. For this case: https://repl.it/repls/WiryOlivedrabInfo, the interpolations should have one more inner indentation
      <a @click="open = !open">
        {{
        open
        ? '[-]'
        : '[+] ' + pluralize(comment.kids.length) + ' collapsed'
        }}
      </a>
  1. For multiple attributes that extend beyond the printWidth, I would prefer this:
<div
  attr="foo"
  attr="foo"
  attr="foo"
  attr="foo"
>bar</div>

over

<div
  attr="foo"
  attr="foo"
  attr="foo"
  attr="foo">bar</div>

(which is less readable IMO)

Question 1: I don't know much about Angular, but which parts are you parsing as interpolations in Vue? For example, in the playground above, it doesn't seem to identify :to as interpolations and use prettier for it:

<router-link
        :to="'/user/' + comment.by + foo + bar + foo + bar + foo + foo + bar + foo + bar">{{ comment.by }}</router-link>
      {{ comment.time | timeAgo }} ago

Question 2: Do you ever foresee yourself implement these Vue style guide points, or should I fork prettyhtml if I want to implement them?

Thanks again for chiming in. This already looks great!

@StarpTech
Copy link
Contributor

StarpTech commented Aug 19, 2018

Hi @octref thanks for the great feedback! I'm free for any change as long as it is reasonable. Just open an issue in prettyhtml and we can work together on a great solution.

Question 1: I don't know much about Angular, but which parts are you parsing as interpolations in Vue? For example, in the playground above, it doesn't seem to identify :to as interpolations and use prettier for it:

I hope I understand your question correctly. Any expression of {{ }} or { } or <% %> inside text nodes are interpreted as interpolation.

Question 2: Do you ever foresee yourself implement these Vue style guide points, or should I fork prettyhtml if I want to implement them?

The first and the last are already covered but the other rules require an application context because I don't know which tag can be used as self-closing tag. IMHO this should be handled by a Vue linter.

I think at first we should focus on a good basic rule set and shouldn't fork it at an early stage.

@StarpTech
Copy link
Contributor

For multiple attributes that extend beyond the printWidth, I would prefer this:

Prettyhtml/prettyhtml#19 I agree 👍

@octref
Copy link
Member Author

octref commented Aug 19, 2018

@StarpTech Sounds good, for Q1, I meant that in this snippet:

<router-link
        :to="'/user/' + comment.by + foo + bar + foo + bar + foo + foo + bar + foo + bar">{{ comment.by }}</router-link>
      {{ comment.time | timeAgo }} ago

What's inside the :to="" is a JS interpolation.

https://vuejs.org/v2/guide/syntax.html#Shorthands

@StarpTech
Copy link
Contributor

StarpTech commented Aug 19, 2018

@octref ok I understand but what's your input and expectation? In my opinion attribute values should remain unaffected. In the current state Prettyhtml handle it as space sensitive content but I'm open for different plugins for vue or angular in order to format such things.

@StarpTech
Copy link
Contributor

@octref Please consider prettyhtml at first as a general formatter. In the second step, when the base is solid and well thought out we can handle framework specific things.

@octref
Copy link
Member Author

octref commented Aug 19, 2018

@StarpTech I would expect, in prettyhtml's current form, to have:

<!-- input -->
<router-link
        :to="'/user/' + comment.by + foo + bar + foo + bar + foo + foo + bar + foo + bar">{{ comment.by }}</router-link>
      {{ comment.time | timeAgo }} ago

<!-- output -->
<router-link
  :to="'/user/' + comment.by + foo + bar + foo + bar + foo + foo + bar + foo + bar"
>{{ comment.by }}</router-link>
{{ comment.time | timeAgo }} ago

There are cases like this, which we can deal with later.

<template>
  <div class="progress" :style="{
    'width': percent+'%',
    'height': height,
    'background-color': canSuccess? color : failedColor,
    'opacity': show ? 1 : 0
  }"></div>
</template>

Please consider prettyhtml at first as a general formatter.

I agree. Kudos to your work! I'd suggest setting up Travis/Codecov/Contributing.md soon. There are many people who would like to contribute: #561. If I do get time I can help a bit too :)

@StarpTech
Copy link
Contributor

@octref the first case of your last example is already covered

@octref
Copy link
Member Author

octref commented Aug 22, 2018

Looking good. I'll take a look this weekend to integrate prettyhtml.

@octref octref mentioned this issue Sep 5, 2018
3 tasks
@maeldur maeldur mentioned this issue Sep 7, 2018
3 tasks
@StarpTech
Copy link
Contributor

@octref any feedback?

@octref
Copy link
Member Author

octref commented Sep 10, 2018

@StarpTech Sorry, had been busy with work. I'm looking at it right now.

@numfin
Copy link

numfin commented Sep 12, 2018

@StarpTech now THATS a killer feature !!!

@alexbudure
Copy link

Can't wait for this!

@StarpTech
Copy link
Contributor

StarpTech commented Sep 17, 2018

@numfin @alexbudure you can already test the formatter on your codebase to provide me feedback.

You can also do it online https://repl.it/@StarpTech/PrettyHtml

@alexbudure
Copy link

@StarpTech Tried it out and it works great so far. Solves a lot of issues I've been having.

How can I integrate this with Vetur?

@StarpTech
Copy link
Contributor

@alexbudure that's a question for @octref.

@alexbudure
Copy link

@octref Any timeline on this? Even a quick integration where you can choose the PrettyHtml instead of JSBeautify would be greatly appreciated!

octref added a commit that referenced this issue Sep 25, 2018
@octref octref closed this as completed in 6e87828 Oct 3, 2018
octref added a commit that referenced this issue Oct 3, 2018
@numfin
Copy link

numfin commented Oct 4, 2018

YEEEEEAHBOOOOYYYYYYYYYYYYYYYYYYYYYIIIIIIIIIIII

@nklayman
Copy link

There is an active PR that adds HTML/Vue support to prettier. It might not be necessary thanks to prettyhtml, but worth a look.

@StarpTech
Copy link
Contributor

StarpTech commented Oct 13, 2018

@nklayman that's right and I'm aware of that from the beginning. I hadn't time to investigate full-time on it and in our company we looked for a fast and maintainable solution.

Before I will repeat myself:

https://github.com/Prettyhtml/prettyhtml#why
prettier/prettier#5259 (comment)

@tarikhamilton
Copy link
Contributor

Is there a reason why Vetur is defaulting to a new library instead of contributing to https://github.com/beautify-web/js-beautify? They've finally found contributors.

@bsaf
Copy link

bsaf commented Oct 30, 2018

Prettyhtml integration is working brilliantly for me, thank you to @StarpTech and @octref for implementing!

@octref
Copy link
Member Author

octref commented Nov 20, 2018

@tarikhamilton

  • js-beautify was not maintained for a long time
  • prettyhtml takes another approach than js-beautify. Similar to prettier, it's more opinionated and outputs better formatting results.
  • I currently do not have time to contribute to either. So we have the option vetur.format.defaultFormatter.html where you can choose either

@qiulang
Copy link

qiulang commented Feb 1, 2019

@octref @StarpTech,
Guys, now we use prettyhtml for vue template, but it just makes my case even worse. Because prettyhtml, prettier and vetur will give 3 different results for my vue files!

Please check my issue the details.

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

No branches or pull requests

9 participants