salsify / ember-exclaim

An addon allowing apps to expose declarative, JSON-configurable custom UIs backed by Ember components
MIT License
14 stars 1 forks source link

Exclaim JSON arguments: shorthand property and component arity #11

Open skarger opened 6 years ago

skarger commented 6 years ago

The goal of this issue is to propose two mutually exclusive API changes, and hopefully choose one of them to pursue.

As it states on the README, in order to reference a component in JSON, Exclaim supports an explicit long form style and well as a shorthand-property style:

For example, the text component in the demo application declares its shorthand property to be content, making this:

{
  "$component": "text",
  "content": "Hello, world!"
}

Equivalent to this:

{ "$text": "Hello, world!" }

The problem is that the shorthand-property style becomes confusing when the component requires multiple arguments. Consider an example playing_card component declared with the shorthand-property style:

{
  "$playing_card": "spade",
  "rank": 9
}

Notice that it's visually unclear that the rank field is related to the Exclaim component. It just looks like another field in the object.

The confusion is partly due to the mixed arguments: the shorthand argument is positional while the remaining arguments are named. In addition, there are visual spacing issues that become evident when the shorthand argument is an object:

{
  "$playing_card": {
    "$bind": "my_hand.high_card_suit
  },
  "rank": {
    "$bind": "my_hand.high_card_rank"
  }
}

JSON objects are often written with line breaks between fields for clarity. In this case though, that also introduces more visual separation between the component name, "$playing_card", and its second argument, "rank".

Given the above, I propose eliminating the hybrid style. I can see two approaches to doing that:

Proposal 1

Proposal 2

{
  "$component": "playing_card",
  "suit": "spade",
  "rank": 9
}

Additional Notes

Both proposals simplify the API because they end up with only one way to declare an Exclaim UI configuration. Also both options remove the confusing hybrid when a component accepts multiple arguments but uses the shorthand-property style.

Both proposals would constitute breaking API changes. While that might be fine, it would be possible to avoid that problem by simply changing the documentation to encourage the chosen path, and allowing the code to continue supporting the old style, perhaps with a deprecation warning.

Personally I would lean towards Proposal 1. In usage so far, I have found the shorthand-property style to read clearly and concisely when it has a solo argument.

Proposal 1 has the added benefit that any given Exclaim declaration only has one top-level key (the $-component name). This is helpful because when JSON is serialized, the field order is not guaranteed by default. Therefore a component's arguments may appear above its name, which is more difficult to read.

In Proposal 1 the example above would become:

{
  "$playing_card": {
    "suit": "spade",
    "rank": 9
  }
}
deverstalmage commented 6 years ago

Personally I've never had any problems reading the shorthand component declarations. Between the two proposals I favor number 2 as it's already supported today without any modifications to the library.

If there is broad consensus that the shorthand formatting is confusing then it seems fine to me to make shorthand an undocumented feature (i.e. remove it from the README and Confluence docs) that we may one day deprecate. Until then though I think we may as well keep the capability around.

skarger commented 6 years ago

Between the two proposals I favor number 2 as it's already supported today without any modifications to the library.

For proposal 1 as well the library could continue supporting the longer style but no-doc it. Rather the docs would say to use an object for the shorthand property if the component needs to reference multiple fields, which works today.

A more involved change would be to eliminate the shorthand property's name. Currently if your shorthand property is named my_value then you can access it in the component as config.my_value.suit, config.my_value.rank. If we started from day 1 with only the shorthand property, probably there would be no explicit name for that argument. Instead it would be available as config.suit, config.rank.