Initialize props pattern #2

[danielamorse]

Jira

• http://vjira2:8080/browse/BDS-536
• http://vjira2:8080/browse/BDS-875

Summary

I took another pass at the component props pattern with feedback from the team.

Note: original PR #1041 was merged and reverted. This is a replacement PR.

Details

I added a new Twig function initialize(_context, schema) and a new utility function buildPropsArray(). initialize() returns an array with two keys:

1. ["props"]: a Drupal Attributes object used for rendering props with keys formatted in kebab-case (includes "attributes").
2. ["data"]: an array of props that includes default values from the schema with keys formatted in snake_case.

The data returned only loops through the schema one-level deep, so props that are multi-level (like author.title) are skipped. Going forward, we want to flatten our component schemas, and so this decision keeps that pattern in mind.

Next steps

• Add function to apply inner/outer class filtering (http://vjira2:8080/browse/BDS-884)

How to test

• Review new pattern and functions. Do you approve of this approach? Any suggestions?

[remydenton]

This is looking good @danielamorse!

I just pushed several commits that mostly rework syntax and some of the logic as well (nothing too drastic). Normally I wouldn't presume to impose my preferred syntax on someone else, but since you're still finding your way around PHP, I just went ahead and did it.

I've still got some thinking and rewriting to do, but in the meantime, a few thoughts/questions:

• What's the rationale for the extra step of the mapInputCaseType() method? Would it make more sense to just use the type constants provided by the CaseHelper library?
  For example, I found myself looking at convertToSnakeCase() and wondering what values it accepted for "type". Instead of making a custom "snakecase", "kebabcase", "camelcase", why not just use CaseHelperFactory::INPUT_TYPE_SNAKE_CASE, CaseHelperFactory::INPUT_TYPE_KEBAB_CASE, and CaseHelperFactory::INPUT_TYPE_CAMEL_CASE directly?
• Though Attributes is sometimes an array, it's not guaranteed to be. Sometimes it's actually an object. I haven't tested yet, but I think that could throw a wrench in what we're doing in buildPropsArray(). I'll see if I can update the syntax.
• I think there are cases where the schema designates something as a string but it actually shouldn't be a prop. A good example in bolt-blockquote is content.
  My solution to this would be to designate variables like that as not just strings. We're already doing that for some components, like link. The idea here is that it's not strictly a string in the same sense that, say, 'size' is-- it's really just something that you need to be able to render in a twig template (and could therefore be a render array).
• On that note, since attributes isn't a string, do we need to explicitly check for it, or can we safely rely on the subsequent check for a objects/arrays to weed it out?

I'll probably keep noodling on this tomorrow, but again, good stuff so far!

[danielamorse]

@remydenton, nice edits to the PHP. It was helpful for me to review and see the improvements to syntax and readability.

• What's the rationale for the extra step of the mapInputCaseType() method?

I was inspired by this example from the docs and liked the idea of passing a short name instead of the full constant, especially if you were calling a converter function, e.g. convertToSnakeCase(), directly rather than via getCaseType(). If we decide to go that route, I'd say drop "case" from the string and just pass "snake" instead of "snakecase". I'm not too attached to this pattern if you'd rather remove it. Either way, we should definitely document the available options somewhere.

• Though Attributes is sometimes an array, it's not guaranteed to be.

That is a very good point. I was assuming it was always an array before we called create_attribute() on it. It would be great if you looked into it, but feel free to kick back to me.

• I think there are cases where the schema designates something as a string but it actually shouldn't be a prop.

As you probably saw currently I'm filtering those props out manually. Yes, it would be better to solve this as a higher-level. My concern with using [string, object, array] is that an object that is not a render array would throw an error if you try to render it, right?

• On that note, since attributes isn't a string, do we need to explicitly check for it, or can we safely rely on the subsequent check for a objects/arrays to weed it out?

Good point. I think the subsequent check would weed it out, but would want to test to be sure. Side note: the other important thing here is that each schema-approved prop is added to the array after attributes has been added so that the schema props always win out.

Thanks for putting in the time to clean this up. Let me know if/when you want some help.

[remydenton]

Phew, okay, I think I've done all I have time for for now. Most significantly, I addressed the attributes coming in as an object issue. You can test by adding this at the top of blockquote.twig:

{% set attributes = create_attribute(attributes|default({})) %}

Before my last commit, that would throw errors about syntax (because the other logic in the twig file assumed this.data was an array when it wasn't). Now, it's always an attributes object and that's the presumption throughout the file. That unfortunately means a slightly more verbose syntax (e.g. this.data.size.value instead of just this.data.size), though I'm not sure there's a good way around that.

Additionally:

• I removed the explicit removal of attributes in buildPropsArray. It should always be an object, so unless the schema is doing something ridiculous like this...

  attributes:
      type: object

  ...it will never get passed through. Make sense? Feel free to kick the tires on that to either confirm or prove me wrong.

• I've left mapInputCaseType(). I think it's fine in light of your example, just that maybe we're trying to put too much in the Utils class and should consider breaking some of this out into a separate class.

• Regarding renderable props:

  My concern with using [string, object, array] is that an object that is not a render array would throw an error if you try to render it, right?

  True, it's possible to pass an array that can't be rendered. But at a certain point, I think we just have to assume the user knows what they're doing. We've partially addressed this in link, for example, by mentioning it in the description: "Renderable text content for the link". My thought is we should do the same here unless you still disagree (could be a good topic for dev huddle if so).

[sghoweri]

Can you please update with the latest code from master + make sure the Travis build is passing?

This is looking a lot better — I really like how the PHP for this is shaping up!!

[sghoweri]

I think that should just about do it for me. We need to update the branch to pull in the latest from master, resolve conflicts, and tweak one or two things based on my comments then I think this is good to approve and merge! 👍🏼

[danielamorse]

I think that should just about do it for me. We need to update the branch to pull in the latest from master, resolve conflicts, and tweak one or two things based on my comments then I think this is good to approve and merge! 👍🏼

@sghoweri I updated with the latest from master and incorporated the changes you suggested. Just one remains, the one about optional dependencies. See comment. Thanks!

[sghoweri]

This looks great. Very nice work @danielamorse -- lets gettr merged!