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

don't include semi in JSX only code blocks in markdown #3196

Closed
chrisdrackett opened this issue Nov 7, 2017 · 14 comments
Closed

don't include semi in JSX only code blocks in markdown #3196

chrisdrackett opened this issue Nov 7, 2017 · 14 comments
Labels
area:multiparser Issues with printing one language inside another, like CSS-in-JS lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:enhancement A potential new feature to be added, or an improvement to how we print something
Milestone

Comments

@chrisdrackett
Copy link

I'm using prettier with a project that uses react styleguidist to document components (docs here: https://react-styleguidist.js.org/docs/documenting.html)

when including just JSX in a codeblock it would be nice if prettier would exclude the leading (or following depending on your --no-semi setting) from the output

Prettier 1.8.1
Playground link

--parser markdown
--no-semi

Input:

```js
<Button>label</Button>
```

Output:

```js
;<Button>label</Button>
```

Expected behavior:

```js
<Button>label</Button>
```
@azz azz added area:multiparser Issues with printing one language inside another, like CSS-in-JS lang:javascript Issues affecting JS type:enhancement A potential new feature to be added, or an improvement to how we print something labels Nov 7, 2017
@azz
Copy link
Member

azz commented Nov 7, 2017

I think this is a little more general. Do we want to detect when the code snippet is a single expression (i.e. not multiple statements), then omit semicolon accordingly?

@suchipi
Copy link
Member

suchipi commented Nov 7, 2017

I guess the question would be if there are any situations where a user writes a single expression for which omitting the semicolon would be undesirable. I can't think of any off the top of my head.

@sapegin
Copy link
Contributor

sapegin commented Nov 24, 2017

So do we have any conclusion here? Is this something you’d like to change? For both semi / no semi options?

@duailibe
Copy link
Member

I believe it's something we want, but there's no one actively working on it to flesh out the details on this behavior (and of course, implement it later).

@suchipi
Copy link
Member

suchipi commented Nov 25, 2017

Maybe this should be framed as a bug? Namely, when using no-semi, we shouldn't put a defensive prefixing semicolon in front of the first statement in the program.

I guess that could break stuff for users who are concatenating scripts together, though...

So maybe it should be:
When using no-semi, and the program is within a markdown code block, don't put a defensive prefixing semicolon in front of the first statement of the program.

But it'd be kinda annoying if you had single-JSX-element blocks interspersed with normal code blocks, and you wanted to use semicolons in the normal code blocks. Because you'd need no-semi just for the JSX element blocks, but you'd want it elsewhere...

So maybe we should special-case "markdown block whose program contains only one statement which is an ExpressionStatement whose expression is a JSXElement" specifically

@lydell
Copy link
Member

lydell commented Nov 25, 2017

@suchipi This isn't specific to the first statement of a program; it applies to any statement starting with JSX. And it's not about concatenating scripts, it's about letting people move the line, or add new lines, without having to think about ASI. See #1433 (comment).

I do understand that it looks strange in documentation, though. It's a very tricky case, with no obvious solution. I guess we'll find one after some more discussion, though.

@lydell
Copy link
Member

lydell commented Nov 25, 2017

I think we may have gotten stuck a bit too much on the leading semi-colon, though. If you re-read this bit of the OP:

when including just JSX in a codeblock it would be nice if prettier would exclude the leading (or following depending on your --no-semi setting) from the output

… you'll see that the actual request is to get rid of the semicolon regardless of the what the semi option is set to.

In other words, the markdown code block is intended to show off just an expression, not a whole program. I guess the OP would want this expression without semicolon as well:

```js
1 + 2
```

However, Prettier can't know the intention – did the user intend to show just an expression or a whole program? We could simply omit semicolons when there's just a single expression in a markdown code bloc, but then I'd bet we'd get issue reports about missing semicolons in those cases.

@suchipi
Copy link
Member

suchipi commented Nov 26, 2017

@lydell I meant that we could treat "first statement" differently than other statements, so it would be a bug in the "first statement" case. I don't think that's the right approach after thinking it over, though.

Good to know about the editor story for moving lines around; hadn't thought about that.

Since this issue is about improving the experience when using react-styleguidist, I think the right approach is to omit semicolons when there's a single JSX element expression in code blocks (but not other kinds of expressions).

@suchipi
Copy link
Member

suchipi commented Nov 26, 2017

To clarify- let's remove semicolons regardless of the semi option in that case, per @lydell

@lydell
Copy link
Member

lydell commented Nov 26, 2017

Spec:

Don’t output semicolon before or after a single ExpressionStatement inside a markdown code block, regardless of the semi option.

I think that will solve the problem. And if somebody complains about a missing semi-colon in that case, we’ll deal with that then. That’s a less worse problem to have than this issue.

@lydell
Copy link
Member

lydell commented Nov 26, 2017

A workaround for now could be to mark the code blocks as json:

```json
<div>test</test>
```

That probably has other side effects, though.

@suchipi
Copy link
Member

suchipi commented Nov 27, 2017

Why not:

Don't output semicolon before or after a single [ExpressionStatement whose expression is a JSXElement] inside a markdown code block, regardless of the semi option.

That would reduce the scope for the potential future "somebody complains about a missing semi-colon" case, and would solve this issue.

@lydell
Copy link
Member

lydell commented Nov 27, 2017

Yeah, that's also an option. Not sure what's best.

@sapegin
Copy link
Contributor

sapegin commented Nov 27, 2017

I’ve tried to implement it (#3330) but I need some help with detecting Markdown.

suchipi pushed a commit that referenced this issue Nov 28, 2017
…ogram (#3330)

* Do not prepend / append with a semicolon the only JSX element in a program

Fixes #3196

* Limit single JSX element without semicolon to Markdown only

* Fix tests
@suchipi suchipi added this to the 1.9 milestone Nov 28, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:multiparser Issues with printing one language inside another, like CSS-in-JS lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:enhancement A potential new feature to be added, or an improvement to how we print something
Projects
None yet
Development

No branches or pull requests

6 participants