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

Line exceeds max length when calling function with single line template literal #1492

Closed
simonkberg opened this issue May 3, 2017 · 1 comment · Fixed by #1497 or renovatebot/renovate#199
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon!

Comments

@simonkberg
Copy link

Input: (with 80 width column indicator)

const test = () => {
  try {
    spinner.succeed(`Detected as running on a ${bold(distro.name)}-based distro.`)
  } catch (e) {}
}
-------------------------------------------------------------------------------^

Output:

const test = () => {
  try {
    spinner.succeed(`Detected as running on a ${bold(distro.name)}-based distro.`)
  } catch (e) {}
}

Expected:

const test = () => {
  try {
    spinner.succeed(
      `Detected as running on a ${bold(distro.name)}-based distro.`
    )
  } catch (e) {}
}

IMO, single line template literals as arguments should be on it's own line if it doesn't fit inside the specified width. Live test case.

@despairblue despairblue added the status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! label May 3, 2017
vjeux added a commit to vjeux/prettier that referenced this issue May 3, 2017
In 1.3.0, we shipped a change that makes template literal always inlined as single arguments of a function. The problem with template literals is that they whitespace is significant so we can't change it. There are two cases:

```js
call(`
  template
  template
`);
```

and

```js
call(
  `template
   template`
);
```

If you always make the same decision to inline, you're going to be wrong for the other use case. The solution that I found that works is to figure out if there's a `\n` before the backtick `` ` ``. If that's the case, then don't inline, otherwise do. We're trying to avoid looking at the source as much as possible but this is one example where we actually don't have a choice if we want to keep the output sane.

1.3.0 made the jest codebase significantly worse because of this. The issue is that once things have been moved, this heuristic won't be able to undo it. So people need to have this fix applied before they run 1.3.0, otherwise it's going to damage their codebase unless they manually change everything back, which is a pain. So I'm going to land this as a hotfix in 1.3.1.

Fixes prettier#1492
vjeux added a commit that referenced this issue May 3, 2017
In 1.3.0, we shipped a change that makes template literal always inlined as single arguments of a function. The problem with template literals is that they whitespace is significant so we can't change it. There are two cases:

```js
call(`
  template
  template
`);
```

and

```js
call(
  `template
   template`
);
```

If you always make the same decision to inline, you're going to be wrong for the other use case. The solution that I found that works is to figure out if there's a `\n` before the backtick `` ` ``. If that's the case, then don't inline, otherwise do. We're trying to avoid looking at the source as much as possible but this is one example where we actually don't have a choice if we want to keep the output sane.

1.3.0 made the jest codebase significantly worse because of this. The issue is that once things have been moved, this heuristic won't be able to undo it. So people need to have this fix applied before they run 1.3.0, otherwise it's going to damage their codebase unless they manually change everything back, which is a pain. So I'm going to land this as a hotfix in 1.3.1.

Fixes #1492
@vjeux
Copy link
Contributor

vjeux commented May 3, 2017

I just pushed 1.3.1 as a hotfix for this. Sorry about it!

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants