With/let/def statement: define variables (suggestion/enhancement)

[lalomartins]

There's this one piece of code where I want to run a method once and bind its result locally. It looks more or less like this:

<For each="item" of={this.state.items}>
  (bunch of stuff)
  <Choose>
    <When condition={this.isExpanded(item)}>
      more stuff
    </When>
    <Otherwise>
      {((summary) => <Choose>
        <When condition={summary}>
          stuff that uses the summary variable
        </When>
        <Otherwise>
          also uses the variable
        </Otherwise>
      </Choose>)(this.getSummaryFields(item))}
    </Otherwise>
  </Choose>
</For>

Kind of makes my eyes bleed, and it's hard to see where the “variable” comes from.

(Sorry if the example is not as minimal as possible, but I wanted to illustrate why I want it to begin with; if it wasn't inside a <For> I could just define it as a const on top of the render method.)

Drawing from the ancient template languages of my young days…

    <Let summary={this.getSummaryFields(item)}>
      <Choose>
        <When condition={summary}>
          stuff that uses the summary variable
        </When>
        <Otherwise>
          also uses the variable
        </Otherwise>
      </Choose>
    </Let>

(Or you could call it <With> or <Def> or a number of other things. Not <Const> though, that would make it look like it's doing something completely different.)

A possible objection is that using prop names this way is iffy, and I guess some are already uncomfortable with the each and index props in <For>. An alternative, arguably more React-y:

<With const={{summary: this.getSummaryFields(item)}}>

It would expand of course to pretty much what I have on top, an in-place function, except no need for trickery with passing things as arguments.

// for clarity, allow me the poetic license of not expanding the other jsx
{(function() {
  const summary = this.getSummaryFields(item);
  return (
    <Choose>
      ...
    </Choose>
  );
})()}

Known issue: expanding it that way means it's limited to containing exactly one child.

[AlexGilleran]

I see the use of it in a big template, but in React what's the advantage of using this over refactoring the summary => bit into its own component?

[lalomartins]

Well maybe I'm doing it wrong but I'm not a huge fan of creating lots of small components.

If I have methods with logic that depend on data on the parent, I'd have to pass those down as props. (Convoluted reason, but it's the case in this real-life example I used above.)

Then, putting an extra component in the same file is bad form but creating a new file for something like this makes things harder to find. And it feels just wrong to me to have a component that will only ever possibly be used in one place.

But mostly, it's the methods thing. And state, and whatever locals I already have at that point. As a rule, if I'm splitting code out into a new component, and I'm passing more than 3 or 4 props, I feel I'm doing it wrong, and try to find a better way.

Ironically, as I write, I'm doing exactly that 😸 I just split one component in 3 because the render method was too long, and ended up passing way too much stuff down. So, there are cases and cases, I suppose.

What I have in mind here is that the block of code really logically/semantically belongs in the same component, but at the same time I only want to call getSummaryFields() once, because it's somewhat costly.

[texttechne]

In my opinion it's kind of best practice to create as many components as possible. It has been pointed out by others, however, that folders with many, many components are quite messy. In the end I think it's a typical problem of one's folder structure... On the other passing props is just a routine piece of work, but it doesn't add extra complexity (so I think this argument is only about lazyiness vs comfort).

In your particular case I think the best way to go forward is to create an own component, as @AlexGilleran suggested.

The new "component" you propose is definitely doable, but I don't see the use case. I've never been in a situation where I wanted to set a new variable in the midst of the template...

[AlexGilleran]

It might be worth pointing out here that jsx-control-statements isn't intended as a templating language to enable big render() functions, it's intended as a way to neaten up the ternary ifs and map statements that you can't avoid using that make it hard to read JSX.

I think this is pretty similar to what https://github.com/wix/react-templates does with scope ?

[martinmacko47]

Sometimes I find the ability to use a local assignment within JSX usefull to make my code cleaner. Not very often, but it's good to have the option available. I made a fork with the With JSX control statement. If interested, I can add docs for the new control statement and create a pull request. If you feel it does not belong directly to jsx-control-statements I can take just the With statement and create a separate package for it, in case somebody want to use it.

Usage of the With statement:

    <For each="item" of={this.props.items}>
      <With value={computeValue(item)}>
        <Choose>
          <When condition={item.path}>
            <Link to={item.path}>{value}</Link>
          </When>
          <Otherwise>
            {value}
          </Otherwise>
        </Choose>
      </With>
    </For>

I can extend eslint-plugin-jsx-control-statements as well, so eslint will know what variables been defined.

[AlexGilleran]

That's really cool, but yeah - I think a separate package is appropriate - it's not really a control statement as such. What do you think @texttechne ?

Will definitely be happy to link to it from our Readme :)

[texttechne]

Nice work. And now I understand the use case.

@AlexGilleran: I think, I would include it as part of jsx-control-statements. While the <With> statement might not be regarded as proper control statement, it is one of the missing pieces to a full fledged templating engine, exactly like conditionals and loops: Variable assignment.

@martinmacko47: The way you've designed the API of the with statement is absolutely fine on its own and very flexible. You can define multiple values in one go: <With attributeName="anyValue" otherName="otherValue">.
Although powerful, it is not consistent with the API of the <For> statement, which uses one attribute to specify the name ("each") and one attribute to specify the value ("of"). More consistency would be necessary to make it part of jsx-control-statements, I think.

[lalomartins]

I kind of had this discussion with myself in the OP 😺 In isolation, clearly I prefer the way @martinmacko47 did it, in fact exactly the way he did it. But to make it more consistent and therefore easier to learn, I proposed an alternative there, with a single attribute, <With const={{summary: this.getSummaryFields(item), displayPicture: this.getDisplayPicture(item)}}>, which is also consistent to the way React does style, even though I don't exactly love that one (style that is).

One could read that and expect they could use also a let={{...} argument which would then be reassignable. But I don't know if that's even a thing with scoping rules… I don't remember off the top of my head what all of this would compile to, but AIUI it's an in-place function call, so a const with the same name would just shadow the other one. There's no space to reassign vars and IMO it would be a bad idea anyway. (Yes if we wanted to I have already thought of a way while I was typing that there isn't one… but no we don't want it.)

Well, I suppose it could compile to do {} statements, but then Babel would compile that to something else anyway, unless you're targeting very recent browsers… and then, well, I'm not sure. Does it use temporary variables? In that case let could make sense, but I still don't think it's worth the confusion.

In that spirit I think const just makes it explicit that this is what it is, and as a welcome side effect it might encourage people to use const elsewhere (pretty much every where) in their code 😉

[AlexGilleran]

Cool beans, I'm sufficiently on the fence about this be swayed towards having it as a part of JCS :).

I actually really like the <With x={y}> syntax though, it's super succinct and neat.

[martinmacko47]

I'm glad you are starting to like the new control statement. I'd prefer adding it to JCS, because if I created a separate package for <With> then I would have to create a separate version of eslint-plugin-jsx-control-statements with a separate version of jsx-control-statements/jsx-jcs-no-undef rule. Which could lead to a mess with so many version of no-undef rule.

As for the syntax, I'm not sure which one is better.

Pros of <With foo={computeFoo()} bar={computeBar()}>...</With> syntax:

• Can assign multiple variables in a single statement
• Looks more natural, like one would expect the assignment to look like

Pros of <With const="foo" value={conputeFoo()}>...</With> syntax:

• Is more consistent with <For> syntax
• Could allow for destructing assignment Destructing For #56 if implemented eventually

Btw, I wouldn't name the attribute const however, because it's not a constant, it's actually a function argument, which may be reassigned with vanilla js within the block.

[martinmacko47]

@AlexGilleran @texttechne Should I create a PR?

[AlexGilleran]

Sure, sorry. I prefer the key={value} syntax but I'm happy with a PR that has either :).

[martinmacko47]

@AlexGilleran So I created PR #63 with key={value} syntax. Tests included.

I'll add with statement to the readme only after I'll extend eslint-plugin-jsx-control-statements as well, so the new statement will work with eslint as well. Then I'll create a separate PR with the updated readme.

[martinmacko47]

@AlexGilleran I've created a PR in eslint-plugin-jsx-control-statements to treat variables defined in With statements as declared. Waiting for a review and a merge there. Then I'll create a PR with our readme update.

[martinmacko47]

Btw, I know nothing about FlowType, so please somebody update jsx-control-statements.flow.js

[martinmacko47]

@AlexGilleran No activity with my PR in eslint-plugin-jsx-control-statements in two weeks. Should I create a PR to update our readme now, or should we still wait for eslint-plugin-jsx-control-statements?

[AlexGilleran]

Yeah just update the readme, ESLint can wait :). If it stays inactive we might have to fork it.

[martinmacko47]

@AlexGilleran Ok. So here is PR #64 with extended readme.

[AlexGilleran]

:D

[AlexGilleran]

Will push a new version to npm a bit later :).

[AlexGilleran]

And published! Thanks @martinmacko47 😁

[martinmacko47]

@AlexGilleran Great! Now, let's hope eslint plugin gets eventually updated as well.

[martinmacko47]

FYI: eslint-plugin-jsx-control-statements was updated today to support With statement