marko-js / marko

A declarative, HTML-based language that makes building web apps fun
https://markojs.com/
MIT License
13.39k stars 645 forks source link

[v4] Rename "ref" to "key" #570

Closed patrick-steele-idem closed 7 years ago

patrick-steele-idem commented 7 years ago

Currently, Marko supports "scoped IDs" in the form of a "ref":

<div>
  <button ref="ok" type="button">OK</button>
  <button ref="cancel" type="button">Cancel</button>
</div>

A nested element/widget can be looked up by a ref:

var okButton = this.getEl('button');

A "ref" attribute results in an ID attribute being assigned to the final element. For example:

<div>
  <button id="w0-ok" type="button">OK</button>
  <button cancel="w0-cancel" type="button">Cancel</button>
</div>

morphdom uses the "id" attribute as a "key" to match up elements during a diff/patch. Therefore, marko is using the "ref" attribute for two purposes:

A "key" is the more general term ("scoped-id" is more appropriate, but too long) and "key" is already well understood to be used for matching up elements during diffing/patching.

As a late breaking change for the Marko v4 release, we want to rename "ref" to "key". We will introduce deprecation warnings, but we do not want to ship the "v4" release with support for "ref".

Thoughts? Concerns? Thanks in advance.

austinkelleher commented 7 years ago

I think this is a good change.

Hesulan commented 7 years ago

I do think "key" is a more accurate description, though I'm not entirely convinced the change is necessary. My only major concern would be possible naming collisions - I can think of plenty of other uses for a custom "key" attribute, but not very many for "ref", which has always been pretty much reserved for widgets anyway. That's not to say I disagree, but I'd like to hear a few more pro's and con's.

If this change is implemented, I would be in favor of providing a temporary / opt-in "ref" -> "key" alias, perhaps in a marko-v3-compat package like what was suggested in #558.

gilbert commented 7 years ago

Does using a ref prevent the element from being messed with during a redraw?

patrick-steele-idem commented 7 years ago

@Hesulan For better or worse, React and all of the React variants have already hijacked the "key" attribute: https://facebook.github.io/react/docs/lists-and-keys.html#keys

The key attribute that we are proposing for Marko would serve the exact same purpose as developers are used to with React. The only difference is that we would also be using the key attribute to allow an element/widget to be referenced by the key (that is, getEl(<key>), getWidget(<key>)). In comparison, React has a separate concept of a ref which started out as a string, but is now a function:

<input
  type="text"
  ref={(input) => { this.textInput = input; }} />

That is another reason we want to get away from ref.

(side note, React requires a lot of code to reference an element... uggh)

@mindeavor

Does using a ref prevent the element from being messed with during a redraw?

No, it doesn't impact rendering. It only impacts how elements are matched up for diffing purposes. DOM elements with the same key are guaranteed to be diffed against each other. Even without specifying a key things will still render correctly, but the developer might notice unexpected behavior (e.g., focus moving to the wrong element or CSS transitions being applied incorrectly).

patrick-steele-idem commented 7 years ago

Also, just to be clear, this would be a simple internal rename from ref to key. It would require minimal code change (although we would have to go fix all of the tests and sample apps :/ ).

Hesulan commented 7 years ago

Ah, forgot about React. I'd say those are pretty good reasons to make the switch.

patrick-steele-idem commented 7 years ago

Thanks for taking this on @austinkelleher ! We want to show a deprecation warning initially and publish a new rc release. We will completely remove support for ref and the deprecation warnings when we do the final v4 release.