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

Support sorting of attributes #22

Closed
5 tasks done
Shinigami92 opened this issue Sep 14, 2019 · 28 comments · Fixed by #23
Closed
5 tasks done

Support sorting of attributes #22

Shinigami92 opened this issue Sep 14, 2019 · 28 comments · Fixed by #23
Assignees
Labels
help wanted We are looking for community help type: feature request Functionality that introduces a new feature

Comments

@Shinigami92
Copy link
Member

Shinigami92 commented Sep 14, 2019

Support sorting of attributes

It would be nice if attributes could optionally be sorted.

Acceptance criterion

  • No breaking changes
  • Minor update
  • Sorting attributes is disabled by default
  • The order of attributes can be changed / customized
  • Tests are implemented
@Shinigami92 Shinigami92 added type: enhancement Functionality that enhances existing features help wanted We are looking for community help labels Sep 14, 2019
@Shinigami92 Shinigami92 self-assigned this Sep 14, 2019
@Shinigami92
Copy link
Member Author

In frameworks like Vue we have property bindings

In example:

//- Button will be enabled
v-btn(color="primary", disabled, :disabled="false") Test

//- Button will be disabled
v-btn(color="primary", :disabled="false", disabled) Test

Currently, the formatter first places an attribute with a colon because the ASCII literal is 58 and occurs before any alphabetical literal

In my opinion, property bindings should take precedence over non-property bindings
Also v-bind: should be prioritized

Need to look into the behavior of v-on and v-slot

@Shinigami92
Copy link
Member Author

Theoretically ready for a beta release

Further thoughts:

  • (maybe) prefer Vue's v-* higher up
  • Prefer Angular's *ng etc higher up
  • Think about ordering of Vue's event binding modifiers
  • Write more tests for Vue bindings
  • Check Vue's new slot syntax in example #header
  • Write tests for Angular bindings
  • Add docu to readme

@Shinigami92 Shinigami92 added type: feature request Functionality that introduces a new feature and removed type: enhancement Functionality that enhances existing features labels May 21, 2020
@SkyaTura
Copy link
Collaborator

I'd like to help with this feature, how could I start?

@Shinigami92
Copy link
Member Author

Thanks :) and yes I could possibly need your (someone else help)
The feature itself is working in this PR, but I was never really satisfied how it works
I want to design an option so you don't need to write like hundreds of different tags
I lastly worked around a year ago on this... yes thats september 16 2019 :D
Since then I thought about to replace this with a regex pattern strategy, but regex patterns are not for everyone and I don't know if I need to support both: plain tags and regex

So all in all, broken down: It's just a problem of how I consume a thing from were I create a compare function returning -1, 0 or 1

And then I want to provide a robust default value for this option to support a common base for vue AND angular together
But I'm only a vue developer myself...

@SkyaTura
Copy link
Collaborator

What about this:

interface pugSortingAttributes {
  sortLeft?: string[]
  sortRight?: string[]
}

where you can specify which starting content should be placed where.

You have 3 positions:

left - middle - right

left and right

Are positioned by the order that you specified in the options.

middle

Sorted ASC

Example

options

const pugSortingAttributes = {
  sortLeft: ['#', ':', 'v-'],
  sortRight: ['@'],
}

input

FooComponent(
  @click="fooAction"
  foo="123"
  :bind="888"
  #child="props"
  v-if="true"
  v-for="foo in bar"
  bar="321"
)

output

FooComponent(
  #child="props"
  :bind="888"
  v-for="foo in bar"
  v-if="true"
  bar="321"
  foo="123"
  @click="fooAction"
)

Testing

const sort = (options) => {
  const {
    sortLeft = [],
    sortRight = []
  } = options || {}

  const sortFirst = [...sortLeft].reverse()
  const sortLast = [...sortRight].reverse()

  return (items) => [...items].sort((A, B) => {

    const [a] = A
    const [b] = B

    const aLeft = sortFirst.findIndex(item => a.startsWith(item)) + 1
    const bLeft = sortFirst.findIndex(item => b.startsWith(item)) + 1
    const left = aLeft - bLeft

    if (left > 0) return -1
    if (left < 0) return 1


    const aRight = sortLast.findIndex(item => a.startsWith(item)) + 1
    const bRight = sortLast.findIndex(item => b.startsWith(item)) + 1
    const right = aRight - bRight

    if (right > 0) return 1
    if (right < 0) return -1

    if (a > b) return 1
    if (a < b) return -1

    return 0
  })
}

// Test Cases

const cases = [

  // Case 0
  {
    input: {},
    expect: {},
    options: {},
  },


  // Case 1
  {
    input: {
      foo: 123,
      'v-for': 555,
      bar: 321,
    },
    expect: {
      bar: 321,
      foo: 123,
      'v-for': 555,
    },
    options: {},
  },


  // Case 2
  {
    input: {
      foo: 123,
      'v-for': 555,
      bar: 321,
    },
    expect: {
      bar: 321,
      foo: 123,
      'v-for': 555,
    },
    options: {
      sortLeft: [],
      sortRight: [],
    },
  },


  // Case 3
  {
    input: {
      foo: 123,
      'v-for': 555,
      bar: 321,
    },
    expect: {
      'v-for': 555,
      bar: 321,
      foo: 123,
    },
    options: {
      sortLeft: ['v-'],
      sortRight: [],
    },
  },


  // Case 4
  {
    input: {
      foo: 123,
      'v-if': 666,
      'v-for': 555,
      bar: 321,
    },
    expect: {
      'v-for': 555,
      'v-if': 666,
      bar: 321,
      foo: 123,
    },
    options: {
      sortLeft: ['v-'],
      sortRight: [],
    },
  },


  // Case 5
  {
    input: {
      '@click': 000,
      foo: 123,
      'v-if': 666,
      'v-for': 555,
      bar: 321,
    },
    expect: {
      'v-for': 555,
      'v-if': 666,
      bar: 321,
      foo: 123,
      '@click': 000,
    },
    options: {
      sortLeft: ['v-'],
      sortRight: ['@'],
    },
  },

  // Case 6
  {
    input: {
      '@click': 000,
      foo: 123,
      ':bind': 888,
      '#slot': 777,
      'v-if': 666,
      'v-for': 555,
      bar: 321,
    },
    expect: {
      '#slot': 777,
      ':bind': 888,
      'v-for': 555,
      'v-if': 666,
      bar: 321,
      foo: 123,
      '@click': 000,
    },
    options: {
      sortLeft: ['#', ':', 'v-',],
      sortRight: ['@'],
    },
  },

]

// "Test Framework"

const isEqual = (a, b) => JSON.stringify(a) === JSON.stringify(b)
const fromEntries = (acc, [key, value]) => ({...acc, [key]: value})
const sortObject = (obj, sortHandler) =>
  sortHandler(Object.entries(obj))
    .reduce(fromEntries, {})
const test = (cases, sortHandler) => cases
  .map((payload, index) => {
    const {input, expect, options} = payload
    const output = sortObject(input, sortHandler(options))
    const equal = isEqual(output, expect)
    return equal
      ? {testCase: index, equal}
      : {testCase: index, equal, expect, output}
  })
  .forEach(item => console.log(item))

test(cases, sort)

I don't know if you already though about this, honestly I should've seen what you already did before, but I forgot it and I didn't want to throw this away 😅

@Shinigami92
Copy link
Member Author

What about v-for, :key and v-if?
I want to force an order of v-col(v-for="x in y", :key="x.id", v-if="x.visible")

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 15, 2020

You could set

const pugSortingAttributes = {
  sortLeft: [
    '#',
    'v-slot',
    'v-for',
    ':key',
    'v-if',
    ':',
    'v-',
  ],
  sortRight: ['v-on', '@'],
}

it would output

FooComponent(
  #child="props"
  v-for="foo in bar"
  :key="foo.id"
  v-if="true"
  :bind="888"
  bar="321"
  foo="123"
  v-on="$listeners"
  @click="fooAction"
)

Not that this would be the best pattern, I'm thinking only about the options specs

@Shinigami92
Copy link
Member Author

Mhhhh
Maybe it needs to be pugAttributesSortLeft and pugAttributesSortRight or I need to find out if "complex" objects are possible as option value 🤔

Could you also please look into angular support? https://angular.io/guide/binding-syntax
So maybe we need to arrange attributes like (click), [(ngModel)], [disabled], *ngIf and *ngFor (and so on)

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 15, 2020

However, if you had a prop named like keyItems it would be sorted incorrectly.

Maybe should be interesting the mixed usage of RegEx, as you proposed, and maybe a object configuration for easing.

Spec

type SortingParameter = string | RegExp | {
  value: string,
  type: 'exactMatch' | 'contains' | 'startsWith' | 'endsWith'
}

interface SortingAttributes {
  sortLeft?: SortingParameter[]
  sortRight?: SortingParameter[]
}

Vue usage

const pugSortingAttributes: SortingAttributes = {
  sortLeft: [
    { value: '#', type: 'startsWith' },
    { value: 'v-slot', type: 'startsWith' },
    { value: 'v-for', type: 'exactMatch' },
    { value: ':key', type: 'exactMatch' },
    { value: 'v-bind:key', type: 'exactMatch' },
    { value: 'v-if', type: 'exactMatch' },
    { value: ':', type: 'startsWith' },
    { value: 'v-bind:', type: 'startsWith' },
    { value: 'v-', type: 'startsWith' },
  ],
  sortRight: [
    'v-on',
    '@',
  ],
}

Angular usage

  • TODO

Workarounds

If these propositions aren't compatible with Prettier's possibilities, we could create some kind of syntax to convert this into a string format, which, however, would make it a lot harder to customize.

Default patterns

There should be an option to toggle between frameworks too, as they may contradict themselves.

@Shinigami92
Copy link
Member Author

I've been thinking about a parameter for specifying the framework for a long time.
So far, however, I've always managed without it. 🤔

@SkyaTura
Copy link
Collaborator

TODO on this issue

Ok, so, just for organize this topic.

Frameworks

  • Gather use cases about Angular Syntax
  • Gather Vanilla use cases
  • Gather more use cases for Vue
  • Decide whether or not to define a framework parameter

Feature

  • Check Prettier compatibility with complex option formats
  • Define the final parameter's structure

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 16, 2020

I've been thinking about a parameter for specifying the framework for a long time.
So far, however, I've always managed without it. 🤔

It would be ignored in case of a custom sorting is defined, so this parameter may be actually pointless.

@SkyaTura
Copy link
Collaborator

@Shinigami92 can I split this issue in 3?

  • We use this one to discuss about sorting in general, plain pug, parameterization, etc
  • Create one to discuss about Vue attributes specificities
  • Create one to discuss about Angular attributes specificities

The reason is facilitate on organize information about syntaxes and use cases and avoid distraction from the main subject of this issue, which, in my opinion, should be the implementation itself.

Sorry for spamming

@Shinigami92
Copy link
Member Author

Shinigami92 commented Sep 16, 2020

Don't be sorry for spamming and yes feel free 😁

You should also have a look into what I already had implemented in #23
It already was working, just didn't liked the attribute structure

You (or I when I have more time e.g. over the upcoming weekend) should mainly investigate deeper into whats possible via prettier options

I'm currently working on new typedefs of these options: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47626/files

@Shinigami92
Copy link
Member Author

@SkyaTura https://v3.vuejs.org/guide/migration/v-if-v-for.html#overview 👀
Things will be much different...

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 18, 2020

So, two things:

  1. This package will increase it's downloads A LOT (at least, it should)
  2. We will in fact need to differentiate which template we would like to use. A +1 to the "framework parameter"

Edit:
Would be interesting to have a rule that forbids v-if usage within v-for at the same level, tho

@SkyaTura
Copy link
Collaborator

About Angular, it would be nice if someone that really uses it took this task, because I really don't know much and I am no having enough time to learn it :/

@Shinigami92
Copy link
Member Author

Shinigami92 commented Sep 19, 2020

I'm thinking about a sorting template kind of thing ("angular", "vue2", "vue3"), but I'm also not sure about that, cause vue2 will somedays be deprecated 🤷‍♂️

Oh! and my formatter is not a linter 😉

Edit: the more I thought about provide a template, the more I think that's not a good idea.
We just should provide example configurations that can be copy pasted 🤷‍♂️
Only downside is a (maybe) huge configuration file .prettierrc.json ... but you already need a .prettierrc.json cause I will never turn on sorting on by default!

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 20, 2020

Edit: the more I thought about provide a template, the more I think that's not a good idea.
We just should provide example configurations that can be copy pasted 🤷‍♂️
Only downside is a (maybe) huge configuration file .prettierrc.json ... but you already need a .prettierrc.json cause I will never turn on sorting on by default!

I agree.

Oh! and my formatter is not a linter 😉

Sometimes I forget this, because I use like as if it was 🙈

@Shinigami92
Copy link
Member Author

Taking your prev example and convert it to a regex array 🤔

const pugSortingAttributes: SortingAttributes = {
  sortLeft: [
    '^#',
    '^v-slot',
    '^v-for$',
    '^:key$',
    '^v-bind:key$',
    '^v-if$',
    '^:',
    '^v-bind',
    '^v-'
  ],
  sortRight: [
    '^v-on',
    '^@'
  ]
}

And cause of prettier limitations I thing it should be:

const pugSortAttributesLeft: string[] = [
  '^#',
  '^v-slot',
  '^v-for$',
  '^:key$',
  '^v-bind:key$',
  '^v-if$',
  '^:',
  '^v-bind',
  '^v-'
];
const pugSortAttributesRight: string[] = ['^v-on', '^@'];

... but what should be done with all the attributes in the middle?

Sort literally?

This could break code in the worst case

//- ALWAYS disabled
v-text-field(:disabled="val", disabled)

//- depends on `val`
v-text-field(disabled, :disabled="val")

Maybe this is also in angular, but I don't know angular+pug

Don't sort at all?

Why are some parts sorted and some parts not 🤔 🤷‍♂️

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 22, 2020

... but what should be done with all the attributes in the middle?

Maybe the pugSortAttributes could define this behavior.

const pugSortAttributes: ('none' | 'ascending' | 'descending') = 'none'
const pugSortAttributesLeft: string[] = []
const pugSortAttributesRight: string[] = []

This could break code in the worst case

I see the example you gave is a design flaw, that shouldn't be there at all.

HOWEVER, if this is a valid use case for someone, it would be simply solved by moving bindings to the dark Right side...

Don't sort at all?
Why are some parts sorted and some parts not 🤔 🤷‍♂️

We'll never know

Maybe sorting in a way that's "visually pretty", like a stair of attributes:

Component(
  simple
  bigAttributes
  evenBiggerAttributes
  makesABeautifulStaircase
  ofAscendingLengthVariables
)

What about const pugSortAttributes = 'ascendingLength'?
~just kidding 😂

@Shinigami92
Copy link
Member Author

@SkyaTura I started to work on the sorting feature!

So there are some changes to the concept:

  • The naming will be pugSortAttributesBeginning and pugSortAttributesEnd
  • I'll skip pugSortAttributes: none, asc, desc for a first release of the feature
  • I'll not provide defaults at all, both (beginning and end) will be empty arrays, and if there are values, it will start to sort with them

@Shinigami92
Copy link
Member Author

@SkyaTura I released a beta: https://www.npmjs.com/package/@prettier/plugin-pug/v/1.7.0-sorting-feature.1

I'm currently testing against a huge organizational repo (~80 pug files each around 300 lines long)

My setup so far:

{
  "arrowParens": "always",
  "bracketSpacing": true,
  "printWidth": 120,
  "semi": true,
  "singleQuote": true,
  "pugSingleQuote": false,
  "trailingComma": "none",
  "tabWidth": 2,
  "useTabs": false,
  "pugSortAttributesBeginning": [
    "^cols$",
    "^v-else$",
    "^v-for$",
    "^:key$",
    "^v-if$",
    "^v-on$",
    "^v-bind$",
    "^v-model$",
    "^name$",
    "^:*label$",
    "^:items$",
    "^:*item-text$",
    "^:*item-value$",
    "^:*item-disabled$",
    "^:*placeholder$",
    "^:*src$"
  ],
  "pugSortAttributesEnd": [
    "^:(?!(loading|disabled))",
    "^target$",
    "^@click",
    "^@",
    "^:loading$",
    "^:disabled$",
    "^data-"
  ]
}

@Shinigami92
Copy link
Member Author

@SkyaTura One problem I encountered so far is:
I tried to order my attributes in a somewhat alphabetical order ignoring colons

e.g.

v-dialog(v-model="csvUploadStart", :persistent="loading", width="500")

// first v-mode
// then others by alphabet...

So to fix this, I currently have to define 26 patterns to order alphabetically ^^'
from ^:*a ... to ^:*z

But I think more and more about to just change my own art of sort :D

@Shinigami92
Copy link
Member Author

Wow, interesting case...

v-text-field(v-model="email", name="email", label="Your email address", filled, required, :rules="...")
v-checkbox(v-model="", :label="$t('...')", color="primary")
v-chip.mr-2.px-1(v-text="$t('...')", color="...", label, outlined, small)
  1. In v-text-field I want a (:)label, everything working here cause there is no color
  2. In v-checkbox I want a (:)label before color. Because it's holding a string value
  3. In v-chip the label is irrelevant and a boolean value and I want color before label. But that's not possible/compatible with 2

🤔

So this is something impossible to configure...

@Shinigami92
Copy link
Member Author

Shinigami92 commented Sep 26, 2020

Closer to perfection... (Vue 2)

{
  "arrowParens": "always",
  "bracketSpacing": true,
  "printWidth": 120,
  "semi": true,
  "singleQuote": true,
  "pugSingleQuote": false,
  "trailingComma": "none",
  "tabWidth": 2,
  "useTabs": false,
  "pugSortAttributesBeginning": [
    "^cols$",
    "^v-else$",
    "^v-for$",
    "^:key$",
    "^v-if$",
    "^v-else-if$",
    "^v-on$",
    "^v-bind$",
    "^ref$",
    "^v-model",
    "^name$",
    "^:?type$",
    "^:value$",
    "^v-text$",
    "^:?label$",
    "^:headers$",
    "^:items$",
    "^:?item-text$",
    "^:?item-value$",
    "^:?item-disabled$",
    "^:?placeholder$",
    "^:?src$",
    "^:?color$",
    "^:?text-color$",
    "^:?icon$",
    "^:?small$"
  ],
  "pugSortAttributesEnd": [
    "^:?hint$",
    "^:?persistent-hint$",
    "^prepend-",
    "^@click:prepend",
    "^append-",
    "^@click:append",
    "^:to$",
    "^exact$",
    "^:(?!(width|height|loading|disabled|data-))",
    "^target$",
    "^:?width$",
    "^:?height$",
    "^@click",
    "^@",
    "^:loading$",
    "^:disabled$",
    "^:?data-"
  ]
}

@Shinigami92
Copy link
Member Author

Would be nice when you also share your sort config after testing the beta release 🙂

@SkyaTura
Copy link
Collaborator

SkyaTura commented Sep 29, 2020

I think I may found a bug 😱

Every time I try to format this file it get a different output:

<template lang="pug">
Foo(
  B
  a
  C
  D
  E
  F
  G
  H
  I
  J
  K
  L
  M
  N
  O
  P
  Q
  R
  S
  T
  U
  V
  W
  X
  Y
  Z
  A
  b
  c
  d
  e
  f
  g
  h
  i
  j
  k
  l
  m
  n
  o
  p
  q
  r
  s
  t
  u
  v
  w
  x
  y
  z
)
</template>

My config to reproduce:

const prettier = {
        trailingComma: 'es5',
        printWidth: 80,
        tabWidth: 2,
        semi: false,
        singleQuote: true,
        endOfLine: 'lf',
        plugins: ['@prettier/plugin-pug'],
        pugSingleQuote: false,
        attributeSeparator: 'none',
        closingBracketPosition: 'new-line',
        commentPreserveSpaces: 'trim-all',
        pugSortAttributesEnd: ['^:'],
      }

Edit

I've made a PR for this problem #120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We are looking for community help type: feature request Functionality that introduces a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants