Card/Renderer unification RFC

[bantic]

Existing methods and inconsistencies

We now have published, quasi-official renderers for dom, html and text

When rendering cards, these renders have different expectations for named hooks that contain setup and teardown methods:

renderer:	dom	html	text
card property name:	"display" and "edit"	"html"	"text"

Since the DOM renderer is used both by the editor (to render cards in either "display" or "edit" mode on the editor surface) and for display-only environment (i.e., rendering cards that are displayed to a user reading an article on a site), the display#setup hook has special semantics for inferring its rendering environment based on the presence of an edit property in the env argument to setup():

let card = {
  name: 'some-card',
  display: {
    setup(element, options, env, payload) {
      if (env.edit) { // <-- the `edit` env property is only provided when rendering
                           // the card for the editor
        // do stuff that only happens in the editor, like showing an
        // "edit" or "remove" button
      } else {
        // do stuff that only happens when displayed to an end-user on a web page
      }
    }
  }
};

Another inconsistency is in the way the setup method works for the different renderers:

renderer:	dom	html	text
setup method 1st arg:	element	buffer	n/a
how to "render" :	create dom nodes, append to element	call buffer.push(string) repeatedly to add HTML strings to the output	return a string (the 1st arg to setup is unused)

Suggestions for unification

• Rename hooks to match renderers: rename the display hook to dom so that it matches the name of its rendering context
• Clarify the difference between dom-rendering for the editor and for display-only situations: Either create new hooks for the editor (unsure what is best, perhaps editor-display and editor-edit) or add a more expressive property to the env like context or isInEditor that a card can use to determine whether to show editor-specific UI (instead of inferring from the presence of env.edit)
• Unify the 1st argument to setup: I propose a buffer object that only has a single method push. In dom rendering scenarios, for each node you wanted to append to the card you'd call buffer.push(node). In html rendering scenarios you'd push a string of HTML. In text-rendering scenarios you'd push a string of text.

feedback welcome @mixonic @rlivsey

[bantic]

Another possible suggestion:

• teardown card elements in the editor automatically: The editor is the only scenario where a teardown method currently applies, because that's the only place where someone would remove a card. The other render contexts (html and text) are display-only scenarios. We should still keep the teardown method to allow for clean-up (of event listeners, etc). But this could lead to an alternative shape for a card that simplifies for all the rendering scenarios. Since teardown is only needed for the editor, perhaps there should be an editor hook with setup, edit and teardown, but only a single top-level function (named dom, text, html) for those other rendering contexts:

let card = {
   name: 'some-card',
   dom(buffer, ...) { ...; buffer.push(myInnerCardDOMNode); },
   text(buffer, ...) { ...; buffer.push('this is the card text'); },
   html(buffer, ...) { ...; buffer.push('<h2>I am a card</h2>'); },
   editor: {
     display(buffer, options, env, payload) {
       // show the "What-You-See" part of WYSIWYG for this card
       // show "edit" button, "remove" button, etc.
     },
     edit(buffer, options, env, payload) {
       // show edit options ("save" button, "add image from disk" button, "remove" button, etc.)
     },
     teardown(returnFromDisplayOrEdit) {
        // remove event listeners, etc
     }
   }
}

[mixonic]

Yeah, there is a lot here to hash out, probably best done in person.

• In theory, an edit mode can exist for any format (for example a native editor). I agree this is only theoretical.
• I would be ok with changing display to dom. Not crazy about it, but it is more consistent.
• I maintain that for simple cards implementing dom twice is super weird. A value on env of isEditingSupported seems ok.
• The original design of persisting the element was that it and the child nodes on it could be re-used in the new mode: You only need to change the parts that are important. I think that was a flawed approach though. The element passed is actually managed by the rendered and thus I don't think we should remove it between mode toggles, however I support clearing all child nodes off it between toggles. 👍
• Not so crazy about dom using push for nodes. IMO it overreaches and the goal of consistency is marred by the abandonment of "normal" DOM API usage.
• text should definitely use a buffer and not the return value.

Since teardown is only needed for the editor, perhaps there should be an editor hook with setup

I'm not as sure this is true. Display mode cards, like Ember components, need teardown as well before rendering the edit mode (or just when destroying the editor instance).

[gpoitch]

• Use a single argument (object) for the setup method. This solves the first argument naming dilemma, allowing you to pass arbitrary properties. Plus, since the api is young, I see the current form as a pain for backwards compatibility as we learn more.
• Agnositic, generic hooks seem better than dom, etc.
  env.edit (or isEditable) seems better than a separate editing hook.
• The return value of the display.setup hook could be used by the renderer to handle whether it appends dom nodes, or appends a string, etc, instead of passing in a parent node/string buffer. That eliminates the need for the buffer.push abstraction. Also solves a separate issue of requiring a wrapper element for cards.

[bantic]

Since teardown is only needed for the editor, perhaps there should be an editor hook with setup

I'm not as sure this is true. Display mode cards, like Ember components, need teardown as well before rendering the edit mode (or just when destroying the editor instance).

@mixonic I think we're in agreement. Cards will only be removed from an edit surface (by postEditor, e.g. or backspace), so only cards that are rendered for an edit surface need a teardown method.

Another thing to think about is what community-created/shared cards would look like. If each of them had to implement all the hooks for all possible render contexts they could get fairly complicated and bloated (for example, to render a card in dom client-side it would be wasteful to ship a card with a lot of unrelated html/editor/text rendering code).
So perhaps cards really only need to implement the setup/etc method for the situation they expect to render in, and you'd have a separate version of the card that you'd pass to the HTMLRenderer, another version to pass to the DOMRenderer, etc. And when using a community card you'd only import the version(s) for the rendering contexts your use case involves (i.e., if you render only HTML strings server side, never text or DOM or iOS, you only import the published html card).

The return value of the display.setup hook could be used by the renderer to handle whether it appends dom nodes, or appends a string, etc

@gdub22 Are you suggesting the renderer would detect what type of thing was returned and do the appropriate thing with it? (i.e., detect an array, assume it's an array of dom nodes, append each dom node, etc). Eliminating buffer.push would be good if possible.

[mixonic]

Cards will only be removed from an edit surface (by postEditor, e.g. or backspace), so only cards that are rendered for an edit surface need a teardown method.

Right, but they need two teardowns. One for edit mode, and one for display. A proposed structure you had above seemed to share the teardown for both modes.

I'm not sure I agree with the basic idea that no other env requires teardown anyway. On Bustle, if you are looking at a post and leave the page you may want to do teardown (like remove yourself from a list of Ember components) when changing pages.

[dtetto]

• All for more unification/streamlining 👍.

• I think separating the card renderers out suggested in @bantic’s last comment makes sense. Combined with @gdub22’s suggestion—returning the card’s whole contents in the appropriate format for the current context—the parent renderer could throw an error if you’ve returned the wrong type (string for DOM, etc) for the current rendering context. Thus needing neither the buffer.push abstraction nor a polymorphic return type.

• For the same reason, I think the editor renderer should especially be separate, rather than as an env.edit flag within a method. The code for editing logic usually runs many times longer than the corresponding display code, and for the display-only bulk of cases, you’d want to be able to import only the display logic (i.e. for Bustle’s frontend).

• This also gives much more flexibility with community cards—I can use a community Google Maps card, but override or extend its editor to fit my bespoke editing surface UI. In fact, many-if-not-most community cards could be display-only cards that simply expect the data hash to contain a particular set of properties.

• I mentioned this when I was building out Typeset cards, but I think that separate “modes” within the editor renderer (i.e. editor.display, editor.edit) is overkill in addition to being inconsistent with the other renderer hooks. First, it’s not how any of our cards work in our feels-pretty-typical use case. The cards are all always “editable” and we control the UI state within our card logic. Even if the UI did call for true edit vs. display states, “display” would just be an import or clone of the DOMRenderer version…and we’d still keep track of the state ourselves. The only difference between the editor and DOMRenderer is that it needs access to some actions (to move, remove, or update the card).

  	setup options	Setup return value
  cardDomRenderer	{ element, data }	null (if manipulating element directly) or array of DOM elements to append to element
  cardEditorRenderer	{ element, data, actions: { moveUp, moveDown, remove, save(data), … } }	null (if manipulating element directly) or array of DOM elements to append to element
  cardHtmlRenderer	{ data }	null or HTML string to append to document
  cardTextRenderer	{ data }	null or text string to append to document

• Per @mixonic’s last comment, this also simplifies the need for the editor to have two separate teardowns. Agreed that teardown does seem useful (e.g., for a community card that attaches window-level event listeners) for at least DOM and editor rendering contexts. (And structuring each card renderer as a hash gives us the future ability to add additional hooks as we invent them.)

[gpoitch]

+1 for separate edit renderers @dtetto, makes a lot of sense.

[rlivsey]

Completely separate renderers makes a lot of sense, I don't see the text card renderer sharing much/any code with the DOM version. Even if they did happen to be used within the same app they'd be in entirely separate contexts.

[mixonic]

Completely separate renderers

I'm not sure what this means, but it spooks me a little. Separate npm packages? A "card" is not much of a useful abstraction if it means you must publish 5 npm packages.

The cards are all always “editable” and we control the UI state within our card logic. Even if the UI did call for true edit vs. display states, “display” would just be an import or clone of the DOMRenderer version…and we’d still keep track of the state ourselves.

I challenge this. Only the listicle items are always in "edit" mode. The other cards leverage edit/display.

[rlivsey]

I'm not sure what this means, but it spooks me a little. Separate npm packages?
A "card" is not much of a useful abstraction if it means you must publish 5 npm packages.

Being separate wouldn't mean it couldn't / shouldn't be distributed in the same package.

Regardless of whether a card implemented all the hooks in one object vs types being separate objects, we'd make different versions with the same name to be used in different situations.

Eg I don't need text rendering in my frontend, but 100% need that on the backend so we can store the text in ElasticSearch. I wouldn't want that to be in the same object because I wouldn't want to distribute it with our Ember app. Ideally when ember-cli supports tree-shaking we could keep it all in the same node package and only pay for the bits we import.

Likewise we would most likely have multiple versions of HTML cards too, as we may render a card differently when including in an HTML email than we would rendering as the HTML for a PDF.

[mixonic]

Some notes from a discussion with Cory. First on packaging:

• Card medias should be expressed in a filename or package name. For example, currently the media for a card is only represented in the format of { html: { setup() {. Instead, we should encourage either a filename: html.js in the mdc-youtube package, or entirely in the package: mdc-youtube-html.
• The package-only solution helps deal with when a package has dependencies for just that version of a card. Say the HTML version requires some node-land specific things, we cannot have webpack loading that stuff up for web-based consumers.
• A package-only ecosystem would be explosive in the number of npm modules. This is also not ideal. Thus encouraging an alternative of package+filename, which may be a simpler way to package up 80% of cards out there.
• Since we're talking about an explosion of NPM modules (even just for sharing Bustle's cards internally), we should decide on strong conventions. We suggest card packages always be in the format of mdc-${card-name} with an optional extension for the media type: mdc-${card-name}-${card-type}.
• Card packages should flag themselves on npm by adding mobiledoc-card to the keywords list.
• We should publish a site ala http://www.emberaddons.com/ that helps to encourage card authors to publish a card for each media, and helps to visually group access to the repos and packages comprising those. (perhaps medias supported in a package should also be provided to the keywords).

Regarding hooks:

• Mobiledoc-kit should clear the children of a card element when that card changes modes
• The return value of all hooks should be an argument passed to teardown. Optional teardown implementation should be preserved for those cases that will require it (removing an observer on the window etc). Using Ember components makes you think teardown isn't needed since Ember tears them down, but there are valid cases without Ember for this hook (in edit and in display).
• Still thinking about: The discussion here is to unify html, text, and dom renderers. This is possible to do (of course), however there may be other medias (iOS, android) that would not have the flexibility needed to match our unifying abstraction. For example if we say all setup hooks are passed buffer with an #append method, we cannot know that later medias will support appending objects to build a user interface. The win we get here (consistency in current media renderers) seem temporary at best.
  • One direction to move for current wins and just use buffer.append, or return an array of things to append.
  • Another is to punt and simply fix the text media to accept a buffer with #push like html has. dom would remain specialized, although we could tweak it to accept a document fragment instead of the element, thus protecting the element from being messed with.
• Mobiledoc dom render needs a destroy lifecycle that calls teardown on rendered cards. This would ensure any listeners setup up for interaction are removed, etc.
• Remove teardown from cards, instead the return value from render or edit will be treated as the teardown for that card and mode.
• Change setup to be render on cards.

Taking a couple ideas from this, we think a minimal card would look like:

// package: mdc-youtube-dom
export default {
  name: 'youtube',
  type: 'dom',
  render(fragment, options, env, payload) {
    let element = document.createElement('div');
    let player = new JQueryPlayer(element, payload.src);
    fragment.appendChild(element);
  }
}

// package: mdc-youtube-dom
export default {
  name: 'youtube',
  type: 'dom', // the renderer using this card can assert it is the correct type
  render(fragment, options, env, payload) {
    let element = document.createElement('div');
    let player = new JQueryPlayer(element, payload.src);
    fragment.appendChild(element);
    return () => { // returning a function takes the place of teardown hooks
      player.destroy();
    };
  },
  edit(fragment, options, env, payload) {
    fragment.innerHTML = "Man I don't implement no edit mode!!";
  },
}

[dtetto]

I challenge this. Only the listicle items are always in "edit" mode. The other cards leverage edit/display.

My mistake. It looks like this is also true of the image-card though—the attribution and caption are always editable—but not the current embed-card. But this actually makes me want to change the embed-card to be more image-card-like—it also ought to allow in-place editing. But even if its UI did toggle between fully independent display and edit states, that logic would feel more natural to me in Ember-land. Seems both easier to grok and more flexible not to make any assumptions about the card editor workflow and instead just expect that the editor modifies and returns the card’s data however it likes.

[mixonic]

Being separate wouldn't mean it couldn't / shouldn't be distributed in the same package.

The core challenge @bantic and I discussed is that node runtime dependencies are not platform specific. So, given that I have a text renderer that uses a node module which has C bindings, I cannot use anything from that package in webpack without including that dep in my build. Even if I don't require the dependency for the part of the package I use, I must include the dependency. Thus in our above summary we talk about allowing the package name to optionally include the "media", or alternatively have it in a file path. If the text renderer specifically requires a c binding, that version can stand alone on npm. However to limit the explosion of modules, we also allow file-names.

This is pretty in the weeds- and we should not explain it this way to newcomers. They should just have a set of conventions to follow.

Ideally when ember-cli supports tree-shaking we could keep it all in the same node package and only pay for the bits we import.

Even tree-shaking does not apply to deps. And we also need the ecosystem to be webpack-friendly.

[mixonic]

it also ought to allow in-place editing

Managing that state on your own is not trivial- For example @ZeeJab and I whipped up a quick card for flower today, and not needing to build the state for editing/display was a great boost to productivity.

Today, you can use a card in edit mode the whole time it is on the page and call save with a silent argument that prevents toggling back to display. I bet most cards will benefit from not needing to write the boilerplate regarding edit/display, but we review it as we write more cards.

[mixonic]

One more item is that the way cards are passed to the mobiledoc kit and to renderers is different due to premature optimization. It should always be the same (an array, I suggest).

[mixonic]

Let's also change all hook payloads to be objects instead of multiple ordered arguments.

[bantic]

I've distilled feedback from here and put together a more formal RFC/checklist in #234.