denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.06k stars 5.14k forks source link

`deno fmt` should not break up binary expressions and keep some call expressions on same line inside template literal expressions #11173

Open ebebbington opened 2 years ago

ebebbington commented 2 years ago

Deno version: 1.11.2

Reproducable from: https://github.com/ebebbington/chatsterisk/blob/destiny/src/server/public/components/Call.ts (the file here has already been formatted)

image

dsherret commented 2 years ago

@ebebbington that link is a 404. Would you be able to update it so it's easier to copy and paste out the code?

Also, what does "not work well with templates" mean in this case and what is your expected output?

I believe you are referring to this minimal example:

`testing testingt tt t test t t t tt t t t t tt t t t ${this.selected.value === ""}`

Formatting like this:

`testing testingt tt t test t t t tt t t t t tt t t t ${this.selected.value ===
  ""}`;

Instead of this, correct?

`testing testingt tt t test t t t tt t t t t tt t t t ${
  this.selected.value === ""
}`;
ebebbington commented 2 years ago

my bad @dsherret! i deleted the branch as I merged it - i've restored it now so that link should work again

"does not work well" as in, the formatting is all over the place

instead of this, correct?

I do understand that there is a column width limit so i'd get at some point the code will be cut off, but if you take a look at the screenshot, it formats so many lines in such an ugly way (which seems like a bug as it doesnt look like code should be formatted that given the context)

dsherret commented 2 years ago

@ebebbington ok, the only thing I see is what I mentioned.

Could you fill in this template?

**Input Code**

```ts
```

**Expected Output**

```ts
```

**Actual Output**

```ts
```
ebebbington commented 2 years ago

Like why ${ on a newline, then computed(() => { on a completely new line, it's not like the column width is 20

Input Code

  template = html`
    <p>Remember to refresh the extensions to make a call!</p>
      <div class="row flex">
        <div class="col-6 text-align-c">
            <p>Select an extension to call from</p>
            <select id="extension-to-call-from" prop:value=${this.#selectedFromExtension}>
              <option value="" selected=${this.#selectedFromExtension.value === ""}>Select...</option>
      ${computed(() => { return html`
        ${this.#extensions.value.map((extension: number) => {
          if (extension === Number(this.#selectedFromExtension.value)) {
            return html`
              <option value=${extension} style="display: none;" selected="true">${extension}</option>
            `;
          }
          if (extension === Number(this.#selectedToExtension)) {
            return "";
          }
          return html`<option value=${extension}>${extension}</option>`;
        })}
      `;
      })}
          </select>
        </div>
        <div class="col-6 text-align-c">
          <p>Select an extension to call to</p>
          <select id="extension-to-call-to" prop:value=${this.#selectedToExtension}>
            <option value="" selected=${this.#selectedToExtension.value === ""}>Select...</option>
      </select>
    </div>
  </div>
`;

Expected Output

Really just the same as the input, but respecting Deno's column width

  template = html`
    <p>Remember to refresh the extensions to make a call!</p>
      <div class="row flex">
        <div class="col-6 text-align-c">
            <p>Select an extension to call from</p>
            <select id="extension-to-call-from" prop:value=${this.#selectedFromExtension}>
              <option value="" selected=${this.#selectedFromExtension.value === ""}>
              Select...</option>
      ${computed(() => { return html`
        ${this.#extensions.value.map((extension: number) => {
          if (extension === Number(this.#selectedFromExtension.value)) {
            return html`
              <option value=${extension} style="display: none;" selected="true">
               ${extension}</option>
            `;
          }
          if (extension === Number(this.#selectedToExtension)) {
            return "";
          }
          return html`<option value=${extension}>${extension}</option>`;
        })}
      `;
      })}
          </select>
        </div>
        <div class="col-6 text-align-c">
          <p>Select an extension to call to</p>
          <select id="extension-to-call-to" prop:value=${this.#selectedToExtension}>
            <option value="" selected=${this.#selectedToExtension.value === ""}>
             Select...</option>
      </select>
    </div>
  </div>
`;

Actual Output

${, computed(() => {, return html all on new lines with different indentation, making it very hard to read

  template = html`
    <p>Remember to refresh the extensions to make a call!</p>
      <div class="row flex">
        <div class="col-6 text-align-c">
            <p>Select an extension to call from</p>
            <select id="extension-to-call-from" prop:value=${this.#selectedFromExtension}>
              <option value="" selected=${this.#selectedFromExtension.value ===
    ""}>Select...</option>
      ${
    computed(() => {
      return html`
        ${
        this.#extensions.value.map((extension: number) => {
          if (extension === Number(this.#selectedFromExtension.value)) {
            return html`
              <option value=${extension} style="display: none;" selected="true">${extension}</option>
            `;
          }
          if (extension === Number(this.#selectedToExtension)) {
            return "";
          }
          return html`<option value=${extension}>${extension}</option>`;
        })
      }
      `;
    })
  }
          </select>
        </div>
        <div class="col-6 text-align-c">
          <p>Select an extension to call to</p>
          <select id="extension-to-call-to" prop:value=${this.#selectedToExtension}>
            <option value="" selected=${this.#selectedToExtension.value ===
    ""}>Select...</option>
      </select>
    </div>
  </div>
`;
dsherret commented 2 years ago

@ebebbington thanks! I'll get this closer to prettier's output. (note that deno fmt doesn't support formatting html in template literals yet)

Also, ${ goes wherever you put it. Otherwise it would change the value of the string.

ebebbington commented 2 years ago

Thanks! I can see why it would change the indentation, because afaik as fmt is concered, its JS code - whereas in my case, im using markup so i'd want certain lines to be indended (eg <p></p> 'within' a <div>)

So that's why i'd understand the formatter pushing lines of code left

Maybe this might be out of scope? It seems quite a unique case

dsherret commented 2 years ago

@ebebbington yeah, it would be nice to format the html within html tagged template literals.

https://github.com/dprint/dprint-plugin-typescript/issues/9

zandaqo commented 2 years ago

Is there any way to avoid new lines after the opening brace (${) and prior to the closing brace (}) at the moment? To give an example, this code from Prettier:

function render() {
  return html`<div class="header">
                 ${isEditable && !isEditing
                    ? html`<mwc-icon-button @click=${this.onEdit}
                      >${pen}</mwc-icon-button>`
                    : nothing}
                    ${isEditing
                    ? html`<mwc-icon-button
                      id="saveconcept"
                      title="save"
                      >${save}</mwc-icon-button>`
                    : nothing}
             </div>`
}

is turned into this by fmt/dprint:

function render() {
    return html `<div class="header">
                 ${
        isEditable && !isEditing
            ? html `<mwc-icon-button @click=${this.onEdit}
                      >${pen}</mwc-icon-button>`
            : nothing
    }
                    ${
        isEditing
            ? html `<mwc-icon-button
                      id="saveconcept"
                      title="save"
                      >${save}</mwc-icon-button>`
            : nothing
    }
             </div>`;
}

those extra new lines and messed up identations seem a bit too much.

raythurnevoid commented 3 months ago

Another experiment:

Formatting with Prettier 👍: image

Formatting with Deno 👎: image

The line width rule is "too strong" in template literals it should be either ignored or applied in a way that makes sure that the closing bracket is not placed on the left of the opening one even if this means that some code goes over the max width

Alternative "acceptable" result 👍: image

Prettier is smart and if you move property1 to a new line it rearranges everything else like in this last screenshot.