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

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

Open
ebebbington opened this issue Jun 29, 2021 · 9 comments
Labels
deno fmt Related to the "deno fmt" subcommand or dprint feat new feature (which has been agreed to/accepted) upstream Changes in upstream are required to solve these issues

Comments

@ebebbington
Copy link
Contributor

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

@ebebbington ebebbington changed the title deno fmt does not wok well with templates deno fmt does not work well with templates Jun 29, 2021
@dsherret
Copy link
Member

dsherret commented Jun 29, 2021

@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
Copy link
Contributor Author

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)

@ebebbington ebebbington changed the title deno fmt does not work well with templates deno fmt produces ugly code when formtting template literals Jun 29, 2021
@ebebbington ebebbington changed the title deno fmt produces ugly code when formtting template literals deno fmt produces ugly code when formatting template literals Jun 29, 2021
@dsherret
Copy link
Member

@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
Copy link
Contributor Author

ebebbington commented Jun 29, 2021

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 dsherret changed the title deno fmt produces ugly code when formatting template literals deno fmt should break up binary expressions and keep some call expressions on same line inside template literal expressions Jun 29, 2021
@dsherret dsherret changed the title deno fmt should break up binary expressions and keep some call expressions on same line inside template literal expressions deno fmt should not break up binary expressions and keep some call expressions on same line inside template literal expressions Jun 29, 2021
@dsherret dsherret added the deno fmt Related to the "deno fmt" subcommand or dprint label Jun 29, 2021
@dsherret
Copy link
Member

dsherret commented Jun 29, 2021

@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
Copy link
Contributor Author

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
Copy link
Member

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

dprint/dprint-plugin-typescript#9

@kitsonk kitsonk added feat new feature (which has been agreed to/accepted) upstream Changes in upstream are required to solve these issues labels Jun 29, 2021
@zandaqo
Copy link

zandaqo commented Nov 18, 2021

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.

@raythurnvoid
Copy link

raythurnvoid commented Feb 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deno fmt Related to the "deno fmt" subcommand or dprint feat new feature (which has been agreed to/accepted) upstream Changes in upstream are required to solve these issues
Projects
None yet
Development

No branches or pull requests

5 participants