Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fixed: #2240 #2245
- Avoid to use riot-observable for the anonymous looped tags
- Improve ~15 the loop performances for the anonymous tags
- Handle properly the `refs` array computation
- Fix the looped removal issue #2240
  • Loading branch information
GianlucaGuarini committed Feb 4, 2017
1 parent d5a99c5 commit 62e5ca4
Show file tree
Hide file tree
Showing 10 changed files with 97 additions and 76 deletions.
50 changes: 22 additions & 28 deletions lib/browser/common/util/tags.js
@@ -1,5 +1,5 @@
import Tag from './../../tag/tag'
import { getAttr, remAttr, createDOMPlaceholder, createFrag } from './dom'
import { getAttr, createDOMPlaceholder, createFrag } from './dom'
import { contains, each, defineProperty } from './misc'
import { isArray, isUndefined, isReservedName, isFunction } from './check'
import { tmpl } from 'riot-tmpl'
Expand All @@ -8,7 +8,6 @@ import {
__TAG_IMPL,
__TAGS_CACHE,
IS_DIRECTIVE,
REF_DIRECTIVES,
RE_RESERVED_NAMES
} from './../global-variables'

Expand Down Expand Up @@ -42,23 +41,6 @@ export function inheritFrom(target, propsInSyncWithParent) {
})
}

/**
* Returns the ref or data-ref value of any dom node
* @param { Object } dom - DOM node we want to parse
* @returns { String|Null } the attribute value if found
*/
export function getRefAttr(dom) {
return getAttr(dom, REF_DIRECTIVES[0]) || getAttr(dom, REF_DIRECTIVES[1])
}

/**
* Remove all the ref directives from a dom node
* @param { Object } dom - DOM node we want to update
*/
export function remRefAttr(dom) {
REF_DIRECTIVES.forEach(dir => remAttr(dom, dir))
}

/**
* Move the position of a custom tag in its parent tag
* @this Tag
Expand Down Expand Up @@ -173,20 +155,33 @@ export function cleanUpData(data) {
* @param { String } key - property name
* @param { Object } value - the value of the property to be set
* @param { Boolean } ensureArray - ensure that the property remains an array
* @param { Number } index - add the new item in a certain array position
*/
export function arrayishAdd(obj, key, value, ensureArray) {
var dest = obj[key]
var isArr = isArray(dest)
export function arrayishAdd(obj, key, value, ensureArray, index) {
const dest = obj[key]
const isArr = isArray(dest)
const hasIndex = !isUndefined(index)

if (dest && dest === value) return

// if the key was never set, set it once
if (!dest && ensureArray) obj[key] = [value]
else if (!dest) obj[key] = value
// if it was an array and not yet set
else if (!isArr || isArr && !contains(dest, value)) {
if (isArr) dest.push(value)
else obj[key] = [dest, value]
else {
if (isArr) {
const oldIndex = dest.indexOf(value)
// this item never changed its position
if (oldIndex === index) return
// remove the item from its old position
if (~oldIndex) dest.splice(oldIndex, 1)
// move or add the item
if (hasIndex) {
dest.splice(index, 0, value)
} else {
dest.push(value)
}
} else obj[key] = [dest, value]
}
}

Expand All @@ -200,9 +195,8 @@ export function arrayishAdd(obj, key, value, ensureArray) {
*/
export function arrayishRemove(obj, key, value, ensureArray) {
if (isArray(obj[key])) {
each(obj[key], function(item, i) {
if (item === value) obj[key].splice(i, 1)
})
let index = obj[key].indexOf(value)
if (~index) obj[key].splice(index, 1)
if (!obj[key].length) delete obj[key]
else if (obj[key].length === 1 && !ensureArray) obj[key] = obj[key][0]
} else
Expand Down
35 changes: 7 additions & 28 deletions lib/browser/tag/each.js
Expand Up @@ -17,8 +17,6 @@ import {
moveChildTag,
getTag,
getTagName,
getRefAttr,
remRefAttr,
arrayishAdd,
arrayishRemove,
makeVirtual,
Expand Down Expand Up @@ -149,13 +147,11 @@ export default function _each(dom, parent, expr) {
parentNode = dom.parentNode,
placeholder = createDOMPlaceholder(),
child = getTag(dom),
refExpr = getRefAttr(dom),
ifExpr = getAttr(dom, CONDITIONAL_DIRECTIVE),
tags = [],
oldItems = [],
hasKeys,
isLoop = true,
hasRefExpr = false,
isAnonymous = !__TAG_IMPL[tagName],
isVirtual = dom.tagName === 'VIRTUAL'

Expand All @@ -165,13 +161,6 @@ export default function _each(dom, parent, expr) {

if (ifExpr) remAttr(dom, CONDITIONAL_DIRECTIVE)

// check if the loop tag has a ref attribute
if (refExpr) {
// detect if the ref attribute is an expression
hasRefExpr = tmpl.hasExpr(refExpr)
remRefAttr(dom)
}

// insert a marked where the loop tags will be injected
parentNode.insertBefore(placeholder, dom)
parentNode.removeChild(dom)
Expand All @@ -180,7 +169,6 @@ export default function _each(dom, parent, expr) {
// get the new items collection
var items = tmpl(expr.val, parent),
frag = createFrag(),
refAttr = refExpr,
isObject = !isArray(items) && !isString(items),
root = placeholder.parentNode

Expand All @@ -204,13 +192,6 @@ export default function _each(dom, parent, expr) {
})
}

if (refAttr && hasRefExpr) {
refAttr = tmpl(refExpr, parent)
}

// reset the parent ref
if (refAttr) parent.refs[refAttr] = []

// loop all the new items
each(items, function(item, i) {
// reorder only if the items are objects
Expand All @@ -232,8 +213,10 @@ export default function _each(dom, parent, expr) {
parent,
isLoop,
isAnonymous,
tagName,
root: dom.cloneNode(),
item
item,
index: i,
}, dom.innerHTML)

// mount the tag
Expand All @@ -249,7 +232,7 @@ export default function _each(dom, parent, expr) {
if (child) arrayishAdd(parent.tags, tagName, tag, true)
} else if (pos !== i && doReorder) {
// move
if (contains(items, oldItems[i])) {
if (contains(items, oldItems[pos])) {
move.apply(tag, [root, tags[i], isVirtual])
// move the old tag instance
tags.splice(i, 0, tags.splice(pos, 1)[0])
Expand All @@ -266,19 +249,15 @@ export default function _each(dom, parent, expr) {
// if the loop tags are not custom
// we need to move all their custom tags into the right position
if (!child && tag.tags) moveNestedTags.call(tag, i)

// update the parent ref
if (refAttr) {
parent.refs[refAttr].push(isAnonymous ? tag.root : tag)
}
}

if (!mustCreate) tag.update(item)

// cache the original item to use it in the events bound to this node
// and its children
tag.__.item = item
tag.__.index = i
tag.__.parent = parent

if (!mustCreate) tag.update(item)
})

// remove the redundant tags
Expand Down
2 changes: 0 additions & 2 deletions lib/browser/tag/mkdom.js
Expand Up @@ -99,7 +99,5 @@ export default function mkdom(tmpl, html, checkSvg) {
else
setInnerHTML(el, tmpl)

el.stub = true

return el
}
5 changes: 3 additions & 2 deletions lib/browser/tag/parse.js
Expand Up @@ -105,13 +105,14 @@ export function parseExpressions(root, expressions, mustIncludeRoot) {
* @param { HTMLElement } dom - dom node to parse
* @param { Array } attrs - array of attributes
* @param { Function } fn - callback to exec on any iteration
* @param { Boolean } isLoop - flag needed for the looped tags
*/
export function parseAttributes(dom, attrs, fn) {
export function parseAttributes(dom, attrs, fn, isLoop) {
each(attrs, (attr) => {
var name = attr.name, bool = isBoolAttr(name), expr

if (contains(REF_DIRECTIVES, name)) {
expr = Object.create(RefExpr).init(dom, this, name, attr.value)
expr = Object.create(RefExpr).init(dom, this, name, attr.value, isLoop)
} else if (tmpl.hasExpr(attr.value)) {
expr = {dom: dom, expr: attr.value, attr: attr.name, bool: bool}
}
Expand Down
10 changes: 9 additions & 1 deletion lib/browser/tag/ref.js
Expand Up @@ -41,10 +41,18 @@ export default {
remAttr(this.dom, this.attr)
} else {
// add it to the refs of parent tag (this behavior was changed >=3.0)
if (customParent) arrayishAdd(customParent.refs, value, tagOrDom)
if (customParent) arrayishAdd(
customParent.refs,
value,
tagOrDom,
// use an array if it's a looped node and the ref is not an expression
null,
this.parent.__.index
)
// set the actual DOM attr
setAttr(this.dom, this.attr, value)
}

this.value = value
this.firstRun = false
},
Expand Down
38 changes: 25 additions & 13 deletions lib/browser/tag/tag.js
Expand Up @@ -76,6 +76,7 @@ export default function Tag(impl, conf, innerHTML) {
isLoop = conf.isLoop,
isAnonymous = conf.isAnonymous,
item = cleanUpData(conf.item),
index = conf.index, // available only for the looped nodes
instAttrs = [], // All attributes on the Tag when it's first parsed
implAttrs = [], // expressions on this type of Tag
expressions = [],
Expand All @@ -86,19 +87,20 @@ export default function Tag(impl, conf, innerHTML) {
dom

// make this tag observable
observable(this)
if (!isAnonymous) observable(this)
// only call unmount if we have a valid __TAG_IMPL (has name property)
if (impl.name && root._tag) root._tag.unmount(true)

// not yet mounted
this.isMounted = false
root.isLoop = isLoop

defineProperty(this, '__', {
isAnonymous,
instAttrs,
innerHTML,
tagName,
index,
isLoop,
// these vars will be needed only for the virtual tags
virts: [],
tail: null,
Expand Down Expand Up @@ -126,6 +128,7 @@ export default function Tag(impl, conf, innerHTML) {
*/
defineProperty(this, 'update', function tagUpdate(data) {
if (isFunction(this.shouldUpdate) && !this.shouldUpdate(data)) return this
const canTrigger = this.isMounted && !isAnonymous

// make sure the data passed will not override
// the component core methods
Expand All @@ -135,9 +138,9 @@ export default function Tag(impl, conf, innerHTML) {
if (isLoop && isAnonymous) inheritFrom.apply(this, [this.parent, propsInSyncWithParent])
extend(this, data)
updateOpts.apply(this, [isLoop, parent, isAnonymous, opts, instAttrs])
if (this.isMounted) this.trigger('update', data)
if (canTrigger) this.trigger('update', data)
update.call(this, expressions)
if (this.isMounted) this.trigger('updated')
if (canTrigger) this.trigger('updated')

return this

Expand Down Expand Up @@ -201,7 +204,7 @@ export default function Tag(impl, conf, innerHTML) {
defineProperty(this, 'mount', function tagMount() {
const _parent = this.__.parent

root._tag = this // keep a reference to the tag just created
if (root) root._tag = this // keep a reference to the tag just created

// Read all the attrs on this instance. This give us the info we need for updateOpts
parseAttributes.apply(parent, [root, root.attributes, (attr, expr) => {
Expand Down Expand Up @@ -237,7 +240,7 @@ export default function Tag(impl, conf, innerHTML) {

if (impl.fn) impl.fn.call(this, opts)

this.trigger('before-mount')
if (!isAnonymous) this.trigger('before-mount')

// parse layout after init. fn may calculate args for nested custom tags
parseExpressions.apply(this, [dom, expressions, false])
Expand All @@ -249,20 +252,23 @@ export default function Tag(impl, conf, innerHTML) {
this.root = root = dom.firstChild
} else {
while (dom.firstChild) root.appendChild(dom.firstChild)
if (root.stub) root = parent.root
}

defineProperty(this, 'root', root)
defineProperty(this, 'isMounted', true)

if (isAnonymous) return

// if it's not a child tag we can trigger its mount event
if (!this.parent || this.parent.isMounted) {
this.trigger('mount')
}
// otherwise we need to wait that the parent event gets triggered
else this.parent.one('mount', () => {
this.trigger('mount')
})
else {
getImmediateCustomParentTag(this.parent).one('mount', () => {
this.trigger('mount')
})
}

return this

Expand All @@ -279,7 +285,7 @@ export default function Tag(impl, conf, innerHTML) {
ptag,
tagIndex = __TAGS_CACHE.indexOf(this)

this.trigger('before-unmount')
if (!isAnonymous) this.trigger('before-unmount')

// clear all attributes coming from the mounted tag
walkAttrs(impl.attrs, (name) => {
Expand Down Expand Up @@ -327,8 +333,14 @@ export default function Tag(impl, conf, innerHTML) {
unmountAll(expressions)
each(instAttrs, a => a.expr && a.expr.unmount && a.expr.unmount())

this.trigger('unmount')
this.off('*')
// custom internal unmount function to avoid relying on the observable
if (this.__.onUnmount) this.__.onUnmount()

if (!isAnonymous) {
this.trigger('unmount')
this.off('*')
}

defineProperty(this, 'isMounted', false)

delete this.root._tag
Expand Down
4 changes: 2 additions & 2 deletions lib/browser/tag/update.js
Expand Up @@ -58,14 +58,14 @@ export function updateDataIs(expr, parent) {
makeReplaceVirtual(expr.tag, ref || expr.tag.root) // root exist first time, after use placeholder

// parent is the placeholder tag, not the dynamic tag so clean up
parent.on('unmount', () => {
parent.__.onUnmount = function() {
var delName = expr.tag.opts.dataIs,
tags = expr.tag.parent.tags,
_tags = expr.tag.__.parent.tags
arrayishRemove(tags, delName, expr.tag)
arrayishRemove(_tags, delName, expr.tag)
expr.tag.unmount()
})
}
}

/**
Expand Down
1 change: 1 addition & 0 deletions riot.js
Expand Up @@ -1294,6 +1294,7 @@ var IfExpr = {
var RefExpr = {
init: function init(dom, parent, attrName, attrValue) {
this.dom = dom;
console.log(dom._tag)
this.attr = attrName;
this.rawValue = attrValue;
this.parent = parent;
Expand Down

0 comments on commit 62e5ca4

Please sign in to comment.