Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

begin implementation #2

Closed
wants to merge 3 commits into from
Closed

begin implementation #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44c54ee
begin cache function
jakeburden Oct 5, 2016
8d3d6b7
clean up for linting
jakeburden Oct 7, 2016
e405d70
clean up for linting
jakeburden Oct 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
const assert = require('assert')

module.exports = cacheElement

// Memoize a bel element
// null -> null
function cacheElement () {
function cacheElement (fn) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps assert()ing the function is actually a function might be useful - a pattern I like is the assert.equal call which takes two arguments + a custom error message that includes the name of the module where the error occured for better traces

const store = {}

return function render () {
const args = Array.from(arguments)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.from() isn't quite compatible with most browsers yet; slicing would probably be better - though on a first render there's no need to diff I reckon so this slicing could be avoided if no prior arguments exist

const argsAreTheSame = JSON.stringify(store.prev) === JSON.stringify(args)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this might be slow for larger data sets; I think we could get away with shallow equality and triple equals. Perhaps we should allow an optional comparison function in the cacheElement call: cacheElement(createElement, compare) so that if a deep comparison or the like needs to be done it's flexible enough to be supported


if (argsAreTheSame) return store.el
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for this to work we can't return the el directly as a Node can't exist in two different DOM trees at the same time. Instead we need to return a proxy element as per this repo - hope this makes sense hah


store.prev = args
store.el = fn.apply(this, args)
return store.el
}
}

10 changes: 10 additions & 0 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
const test = require('tape')
const cacheElement = require('./')

test('cache', function (t) {
var render = cacheElement(function (el) {
return el
})

t.same(render('alice'), 'alice')
t.same(render('alice'), 'alice')
t.same(render('bob'), 'bob')
})

test('should assert input types', function (t) {
t.plan(1)
t.throws(cacheElement)
Expand Down