fix: TS to JS conversion - Ability to spread props within components & allow for empty tags (fragments)

[jpuzz0]

Closes #2815

[patternfly-build]

PF4 preview: https://patternfly-org-pr-2816-v4.surge.sh/v4

[nicolethoen]

is it possible that sometimes it needs the curly braces and sometime it doesn't?

[jpuzz0]

This part of the conversion is specifically for attribute spreading within JSX. I can't think of an example where we would not need the curly braces for this circumstance. I did test that spreading elsewhere in JSX does not enter this block of code, where say if you had something like <div>{[...someArray, ...someOtherArray]}</div>, this is not impacted.

[nicolethoen]

does this still parse React.Fragment correctly?

[jpuzz0]

I did try both, and both are still working. There is an additional block for using JSXFragments where React.Fragment is handled already;

patternfly-org/packages/ast-helpers/stringify.js

Line 129 in 8da725b

JSXFragment(node, state) {

[nicolethoen]

I tested this in my react workspace - it LGTM

[mturley]

This AST stuff is largely dark magic to me, so I hesitate to say I'm 100% confident in any changes here (when this code was originally written, was it just never tested? in what circumstances would it have worked correctly without the curly braces I wonder?)

But, I tested a bunch of variants of spread syntax and they all seem to work fine for me, so ¯\_(ツ)_/¯ LGTM

[DaoDaoNoCode]

Leave a small comment here. Otherwise, all the fixes look good to me, nice job! @jeffpuzzo

[DaoDaoNoCode]

Would it be better to put the ( here inside the if statement below? like:

state.write(' {...');
if (node.argument.type === 'LogicalExpression') {
  state.write('(');
  ......
  state.write(')');
}
......
state.write('}');

We only need the brace when there is a logic expression like && or ||, and in this way, the converted JS code will look better.

[DaoDaoNoCode]

Sorry for not explaining clearly. The state.write('('); should be above this[node.argument.left.type](node.argument.left, state); and the state.write(')'); should be below this[node.argument.right.type](node.argument.right, state);, so it will wrap the whole expression in this way. And we should better keep these 2 state.write(' '); here because we still need the space between the props and the logical operator. So the whole if statement should be like:

if (node.argument.type === 'LogicalExpression') {
  state.write('(');
  this[node.argument.left.type](node.argument.left, state);
  state.write(' ');
  state.write(node.argument.operator);
  state.write(' ');
  this[node.argument.right.type](node.argument.right, state);
  state.write(')');
}

Or you can even combine the center 3 state.write together to be like:

if (node.argument.type === 'LogicalExpression') {
  state.write('(');
  this[node.argument.left.type](node.argument.left, state);
  state.write(` ${node.argument.operator} `);    // or state.write(' ' + node.argument.operator + ' '); 
  this[node.argument.right.type](node.argument.right, state);
  state.write(')');
}

The situation in this if statement is not very commonly seen, many generators don't even consider it, but it's great to keep it here. Hope this makes sense 🙂

[DaoDaoNoCode]

LGTM, nice job, thanks! 😁 @jeffpuzzo

[evwilkin]

LGTM - thanks!