Pass cardOptions to custom renderers

[bantic]

Passes cardOptions as the final argument to markupElementRenderer
and sectionElementRenderer.

[gpoitch]

What is the advantage of this over this #34?
IMO more arguments creates compatibility problems when features are changed/added, and there is already a departure in the signatures of the 2 custom renderers.

[mixonic]

@gdub22 this is mostly at my nudging, though @bantic and I talked it over. There are a couple things at play.

Using the context of a function as an input to that function feels incorrect. For example a naive developer could easily write:

new MobiledocDOMRenderer({
  sectionElementRenderer: {
    p() {
      let tagName = this.someLogicToGetTagName();
      return document.createElement(tagName);
    }
  }
});

What is this? For a first time reader of this code there isn't any reason to presume it is the renderer. This would be much clearer:

new MobiledocDOMRenderer({
  sectionElementRenderer: {
    p(tagName, dom, {renderer}) {
      let tagName = renderer.someLogicToGetTagName();
      return document.createElement(tagName);
    }
  }
});

Now you know exactly what object is providing someLogicToGetTagName. You could also re-write this with a fat arrow:

new MobiledocDOMRenderer({
  sectionElementRenderer: {
    p: (tagName, dom, {renderer}) => document.createElement(renderer.someLogicToGetTagName())
  }
});

And the change from a function to a fat-arrow doesn't change what arguments you have available. We've avoided using this, especially on user-facing hooks, pretty extensively elsewhere in the mobiledoc codebases.

Passing the renderer wholesale feels incorrect. We're concerned that passing the renderer (as this or as an arg) will encourage devs to interact with it during runtime. The renderer may be tracking state or may change the timing of certain operations in the future. Mucking with the internals would be somewhat encouraged by passing the whole object in.

Instead we'd like to continue opting in to features as needed. For example in #34 the canonical use-case is access to cardOptions. That need actually highlights that we should consider renaming cardOptions to something more generic (since these hooks are not cards) or simply teach devs to close over values they need. For example:

const host = 'http://google.com'
new MobiledocDOMRenderer({
  cards: [someCard, anotherCard],
  cardOptions: {host},
  markupElementRenderer: {
    A(tagName, dom, attrs) {
      let element = document.createElement('a');
      element.setAttribute('href', [host, attrs.href].join('/'));
      return element;
  }
});

This seems seems almost more reasonable than passing cardOptions into the renderers. Unlike cards and atoms, we expect markup and section renderers are defined inline where they can close over variables.

But, that said passing cardOptions seems ok. We would like to continue opting in to passing parts of the dom renderer into the markup and section renderers instead of presuming all of it will be present though. A good refactor for the APIs would perhaps be:

  markupElementRenderer: {
    A: (tagName, attrs, {cardOptions, dom}) => elementForA
  }

That would avoid the constant shifting of arguments. We still have some play to change things being pre-1.0, and it seems ok to iterate as we learn instead of using this as an arg to avoid breaking some existing consumers.

[gpoitch]

Great, thanks for the explanation. Typically, just closing over some options works fine. The need to access the options outside of that scope arose when I wanted to reuse the same customized dom-renderer with different 'host' options for example.

Also note that I still need to access the whole renderer instance from within cards. For example, in a listicle-card, you'd want to render the body contents with the parent renderer's options. I currently hack around that by closing over and creating a reference to the renderer itself as a card option.

This LGTM, my main concern was the api for these renderers is becoming argument soup and a potential compatibility nightmare. One hash argument is the way to go for flexibility and self documentation.