JSX - Add toggle for FB translation pipeline compatibility

[karl]

Extracted from #1831

Currently we don't break between text and tags/expressions when there is no existing white space. This is not the ideal behaviour in most cases, but was added to make Prettier compatible with the Facebook translation pipeline (see #1581 (comment) for more detail).

With translation pipeline compatibility:

<div>
  before<div>
    <a>content</a>
  </div>after
</div>

Without:

<div>
  before
  <div>
    <a>content</a>
  </div>
  after
</div>

It would seem sensible to make the behaviour toggle-able and disabled by default. This way projects that don't need compatibility with the Facebook translation pipeline (which should be most of them) can have slightly nicer formatting.

Having compatibility turned off should make it easier to improve the formatting in #2231

[vjeux]

This is not the ideal behaviour in most cases

I would like to challenge this. So far the only cases I've seen are with ( and ) or , and I argue that it makes sense to be kept next to it. Do you have other cases in mind?

[karl]

That would be a very sensible challenge to make!

If I want to push for this change I definitely need to be able to justify it. I had a little bit of time to play around this change (and the formatting change proposed in #2231) and naively following both proposals did lead to some less than ideal behaviour!

My approach with this is going to be to investigate some options and spike some code, I'll create PRs for any spikes that seem promising so we can discuss specific approaches.

If we end up keeping the current behaviour I'm happy with that, I created this issue to make sure the discussion in #1831 didn't get lost.

[rattrayalex]

Hmm, the example @karl included definitely looks better to me without the compatibility.

Detecting punctuation might be possible but sounds hard too:

<div>
  I am sad :-(<Icon />
</div>
// should be broken into:
<div>
  I am sad :-(
  <Icon />
</div>
// ... even though it has a `(` before the jsx element

... on the other hand, I guess it's probably not very common to have words directly next to JSX elements without a space – that's only useful if you want part of a word to be formatted differently, or if you don't care if there's a space or not, eg:

<h3>
  My Heading
  <span class="pull-right"><Icon /></span>
</h3>

which probably isn't a case worth optimizing for anyway.

So I think I might side with @vjeux on the compatibility behavior being ok as the only option.

[vjeux]

I feel like my proposal to respect what the user wrote would solve both use cases. It's also edge-casey enough that it shouldn't trigger a lot in practice.

[rattrayalex]

It's always a bummer to give up consistency... it's an easier argument to make in JSX, where whitespace is technically part of the AST, but still a bummer.

Regardless, would that imply that these two examples would parse differently?

<p>
  hello
  <span> world </span>
  how are you?
</p>

<p>
  hello<span> world </span>how are you?
</p>

because it would feel pretty arbitrary to me to have that not collapse, where something like this would:

<p>
  hello{" "}
  <span>world</span>{" "}
  how are you?
</p>

[vjeux]

-        Add
-        <fbt:param name="number">{value}</fbt:param>
-        days
+        Add<fbt:param name="number">{value}</fbt:param>days

I think that it's going to break fb :( Because there's no more space anymore, it means that there's not going to be any space anymore. So my suggestion to keep spaces the way they were is actually a requirement :x

To be honest, I do think that it's not such a bad thing to leave it the way people wrote it in the first place.

[vjeux]

We could also scope it to tags that are named fbt, so it doesn't pollute the rest of the system if you want.

[karl]

I've slowly been coming to the same realisation that the current master isn't going to work for FB. I'll have a look and see what the quickest fix is to unblock this.

Once we've got the release out that will give us time to investigate other approaches without a pressing time constraint.

[vjeux]

If you want to go through the detection route, <fbt> and <fbt:something> are the two patterns that need to be supported. For a current exhaustive list: <fbt:enum>, <fbt:param>, <fbt:plural>, <fbt:same-param>, < fbt:pronoun>. But we are likely going to add more in the future, so it would be better to whitelist everything that starts with fbt:

Thanks so much for going through it, I know it sucks that we have a custom transform that's not spec compliant :(

[karl]

No worries! There is always stuff like this when a spec meets real world use 😀