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

Feature request: Add a willRender method to (Soy)Components #178

Closed
yuchi opened this issue Dec 13, 2016 · 22 comments
Closed

Feature request: Add a willRender method to (Soy)Components #178

yuchi opened this issue Dec 13, 2016 · 22 comments

Comments

@yuchi
Copy link
Contributor

yuchi commented Dec 13, 2016

In JSX Components there’s a simple moment to compute values to be used in the actual rendering logic:

class MyButton extends JSXComponent {
  render() {
    const className = this.props.primary ? 'btn btn-primary' : 'btn';
    return <button class={ className }>Hallo</button>;
  }
}

This is completely missing in Soy components, and that means that components’ properties more or less need to be the ones you use in the Soy template.
Since in Soy is not possible to call functions to manipulate the data, this makes difficult to have properties that depend on the value of another one.

Examples include:

  • filtered, split or manipulated arrays;
  • transform a file size (in bytes) in a human readable string (“23GB”)

The proposal is to add a new willRender method (similar to React’s componentWillUpdate) that can be used to update the properties passed down to the template.

{template .render}
  <span>The file is {$humanReadableFileSize} big</span>
{/template}
class FileInfo extends Component {
  willRender() {
    return {
      "humanReadableFileSize": humanize(this.fileSize)
    };
  }
}
FileInfo.STATE = {
  "fileSize": {
    value: 0
  }
};
Soy.register(FileInfo, templates);
@mthadley
Copy link

This would be great to have, and I see a real need for this as well.

I've seen sync methods used to try and emulate this behavior in the past, but it is certainly not as efficient, since it means that the component will render at least twice. Once for the first property change, and then again for the derived property after the sync method was called.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 13, 2016

Exactly. The price to pay if obviously a harder process to keep browser and server code in sync

@yuchi
Copy link
Contributor Author

yuchi commented Dec 13, 2016

I’m implementing it, but I took a different approach.

Mutability

First I don’t like having a method that can mutate the object that will be sent to the template. I’d prefer the method to act as if data would be immutable.
We have three options:

  1. the method gets passed the real template data object, and it can (should) mutate;
  2. the method gets passed the real template data object, but must return what becomes the actual data passed to the template — it’s up to the author to choose to either
    • create a new object mixing in the original one with custom keys
    • mutate the original one or
    • just pass new data;
  3. the method gets passed the real (or a clone) template data object and what the method returns is then mixed with the template data and passed to the template.

I personally prefer (and implemented) option N. 2.

Method Name

To call the component method willRender it should be a Lifecycle event, and available to all Metal components, not just Soy. But the concept of template data has no meaning on JSX, so arguments would be renderer specific, and return type too.

While I’d like to have such a lifecycle event there would be some confusion on what you can actually do in it since it’s renderer specific. Also it could cause authors to think that they should use this to compute properties on JSX components, which is a Very Bad Idea™.

In my final implementation in fact it’s named getTemplateData.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 13, 2016

@mairatma, @mthadley what do you think?

yuchi added a commit to yuchi/metal.js that referenced this issue Dec 13, 2016
While in JSXComponents there’s a place to compute derived values from the
state just before rendering (that’s the "render" method), on Soy components
there’s no way to do the same.

This change makes the Soy renderer to call a "getTemplateData" method on the
component passing the data that would be used for the template and trusting
the implementation to do one of the following:
- mutate the original template data and return it;
- mutate the original template data and return undefined;
- create a new object and return it (eventually mixing in the original data).

Fixes metal#178
@mthadley
Copy link

@yuchi I think this makes a lot of sense.

As far as mutability goes, my preference is to avoid it when possible, but I realize that it can be a benefit for performance, so having the option is nice. I have a feeling that option three might be the most intuitive for most metal developers though.

There are a few things to consider with this new feature as well. The developer needs to be aware that not all of their template data is described in their STATE configuration. In addition (like you mentioned), they need to make sure that their getTemplateData logic remains consistent with their server side.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

I have a feeling that option three might be the most intuitive for most metal developers though.

So the implementation will look like this:

if (component.getTemplateData) {
    data = object.mixin({}, data, component.getTemplateData(data));
}

It would look bad in my opinion if authors would want to remove properties from the data, but since Soy templates have strict requirements regarding what you pass to them it’s almost impossible that someone will ever want to do such a thing. So I’m ok with.

The developer needs to be aware that not all of their template data […]

If we named the method getAdditionalTemplateData it would be clearer and tingle the eyes of readers, but you could potentially change state values so “additional” is not good enough.

We could change the implementation so that only additional fields are kept:

if (component.getAdditionalTemplateData) {
    data = object.mixin({}, component.getAdditionalTemplateData(data), data); // the order!
}

But this requires some analysis. I think @mairatma and @eduardolundgren need to chime in.

[…] make sure that their getTemplateData logic remains consistent with their server side.

I though a little about this. I don’t think this is going to be a problem.
We already have a single point that defines what you really need and that’s the Soy template.

In a well written component you would write the Soy template to be as “ignorant” as possible and then have the controllers do the hard work.
If in my view I need to show only a filtered list of elements or an humanized version of a value, then I’d need to write the code for client and server anyway.

@mthadley
Copy link

I think you addressed those concerns well. The SoyDoc in your template is essentially your contract and that is defined in one place. The developer needs to just make sure that they satisfy the behavior that goes along with that, wherever that data is fulfilled. In the end I agree with you though, it should be fine.

@mairatma
Copy link
Contributor

I think I like option 2 better as well, as that avoids us having to object.mixin all the time. Besides, that data object is temporary, so it shouldn't be risky to let the user change it if they decide to. This way they can choose not to though, like you've explained.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

Actually the number of calls to object.mixin or Object.assign or { ...rest, blabla } will be more or less the same. Have a look at a typical implementation of getTemplateData with option 2:

class MyButton extends Component {
  getTemplateData(({ name, ...rest }) {
    // one is here in the form of a spread object
    return { ...rest, name: `Super ${name}` };
  }
}

@mthadley
Copy link

My guess is that it will mostly be used to add additional data, so that means either mutating the data object or calling mixin. So option three might cut down on the boilerplate if we value immutability and expect that most developers would be calling it anyways. However if we expect that they will usually mutate it, then option two makes more sense to me.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

@mthadley Exactly.

@mthadley
Copy link

There's also the initial "gotcha" moment when the developer forgets to mixin the original data when adding their extra stuff on top.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

Since @eduardolundgren proposed some names I’m gonna do a small survey here.


Vote with 👍 if you like a name, 👎 if you find it confusing.

Votes are closed at Thursday 15, 4pm PST.

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

beforeRender

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

formatState

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

mutateState

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

prepareState

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

prepareStateForRender

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

willRender

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

getTemplateData

@yuchi
Copy link
Contributor Author

yuchi commented Dec 14, 2016

prepareTemplateData

@yuchi
Copy link
Contributor Author

yuchi commented Dec 16, 2016

Votes are closed!

Option 👍 😕 👎
beforeRender 0 1 0
formatState 2 0 0
mutateState 0 0 3
prepareState 0 0 0
prepareStateForRender 5 0 0
willRender 1 0 0
getTemplateData 0 0 0
prepareTemplateData 0 0 0

Thanks to this completely useless table we can now confirm the exit polls.
With a huge distance from runnerups wins prepareStateForRender with 5 👍s!!

(I’ll make a PR in a moment)

yuchi added a commit to yuchi/metal.js that referenced this issue Dec 16, 2016
While in JSXComponents there’s a place to compute derived values from
the state just before rendering (that’s the "render" method), on Soy
components there’s no way to do the same.

This change makes the Soy renderer to call a "prepareStateForRender"
method on the component, passing the data that would be used for the
template and trusting the implementation to do one of the following:
- mutate the original template data and return it;
- mutate the original template data and return undefined;
- create a new object and return it (eventually mixing in the
  original data).

Fixes metal#178
yuchi added a commit to yuchi/metal.js that referenced this issue Dec 16, 2016
While in JSXComponents there’s a place to compute derived values from
the state just before rendering (that’s the "render" method), on Soy
components there’s no way to do the same.

This change makes the Soy renderer to call a "prepareStateForRender"
method on the component, passing the data that would be used for the
template and trusting the implementation to do one of the following:
- mutate the original template data and return it;
- mutate the original template data and return undefined;
- create a new object and return it (eventually mixing in the
  original data).

Fixes metal#178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants