choo components #639
choo components #639
Conversation
index.js
Outdated
| state.cache = renderComponent | ||
|
|
||
| function renderComponent (Component, id) { | ||
| assert.equal(typeof Component, 'function', 'choo-component-preview: Component should be type function') |
There was a problem hiding this comment.
I guess this is no longer a preview right?
component/cache.js
Outdated
| assert.equal(typeof state, 'object', 'ChooComponentCache: state should be type object') | ||
| assert.equal(typeof emit, 'function', 'ChooComponentCache: state should be type function') | ||
|
|
||
| this.cache = lru || new LRU(100) |
There was a problem hiding this comment.
An LRU cache would start evicting components that are still on the page if you have a lot of them (eg. 120). I think components that are still mounted should always stay in the cache. Maybe we can have two caches, one plain object .mounted, and one LRU (or user provided) .unmounted. Then we can use on-load to move components into the unlimited .mounted cache on load, and move them to the bounded .unmounted cache on unload.
There was a problem hiding this comment.
simpler is probably better, and i like your suggestion @goto-bus-stop, but here's one other idea: i wonder if it makes sense to have separate caches per component class? i guess it would just be nice not to thrash header/footer/button singleton components if an app happens to have like 1000 tweet component instances...
There was a problem hiding this comment.
That would help too! Id still be worried about the tweets though. as I understand it a list of 100+ tweets would end up recreating and evicting instances during every render even if their content didn't change...
There was a problem hiding this comment.
The onload idea might be pretty difficult to implement in a generic way, though, I'm not sure.
There was a problem hiding this comment.
Id still be worried about the tweets though
If people are thinking of creating lots and lots of elements, we could recommend they create a separate cache for this specifically. If we expose the cache instance, they'll have the flexibility of choosing elements and such.
There was a problem hiding this comment.
+1 on what @yoshuawuyts said but to make that easier couldn't we allow for the third argument (lru) to be a number and forward that to nanocache? So if you don't have any preference as to what lru cache to use, but just want to allow for a lot of instances you'd do
var manyTweetsCache = new ComponentCache(state, emit, 250)Also, most classes in the choo stack allows you to create instances without new using an instanceof check and calling new for you if needed. To maintain a uniform api, shouldn't we do that here as well?
example/components/footer/index.js
Outdated
| module.exports = class Footer extends Component { | ||
| static identity () { | ||
| return 'footer' | ||
| } |
There was a problem hiding this comment.
the identity methods are no longer used, right? they are also defined in header and in info
|
Some more eyes on this would be great if people have time! cc/ @choojs/trainspotters |
|
Not to derail but would this make more sense as a middleware rather than shipping with core? |
|
@ungoldman First-class support for components in the framework helps tremendously with the accessibility effort. Having both |
|
@ungoldman yep, what @jondashkyle says. Components have shown to be a requirement people have in almost every project. Being able to use them straight from core lowers the barrier, and allows us to assume people can build on top of it in every project. |
|
I'm ok with this, but I think docs should also be updated, to reflect the use of components. |
|
ping @tornqvist @bcomnes @toddself your input on this patch would be greatly appreciated 🙏 |
| // properties that are part of the API | ||
| this.router = nanorouter() | ||
| this.emitter = nanobus('choo.emit') | ||
| this.emit = this.emitter.emit.bind(this.emitter) |
There was a problem hiding this comment.
Removing this.emit breaks _prerender. Making sure that emit is a reference to the same function on every render (by referring a bound method on self) really helps with shallow diff of arguments. Also saves memory not having to allocate a new function on every render.
There was a problem hiding this comment.
Oopts, that was totally accidental. Great catch!
There was a problem hiding this comment.
Fixed & added regression tests ✨
yoshuawuyts
force-pushed
the
choojs-components
branch
2 times, most recently
from
bccc530
to
c1f621d
Compare
component/cache.js
Outdated
| if (!el) { | ||
| var args = [] | ||
| for (var i = 0, len = arguments.length; i < len; i++) { | ||
| args.push(arguments[i]) |
There was a problem hiding this comment.
We should probably drop Component and id from the args here (i.e. start iterating on 2). They are both prefixed on the next line anyway and by doing this we'd be offsetting the actual constructor arguments by an additional two.
This is what we're doing now:
class Foo extends Component {
constructor (id, state, emit, the_class, id, actual_constructor_argument) { … }
}
|
Merged in @tornqvist's feedback. Thinking of merging this in 24 hours, and releasing it as a pre-release. Then follow up with a proper release next week. I think it's time to move this forward. Please do chime in if there's any problems left with the implementation. If not, we'll be able able to try it out from a pre-release soon! 😁 |
component/cache.js
Outdated
| assert.ok(this instanceof ChooComponentCache, 'ChooComponentCache should be created with `new`') | ||
|
|
||
| assert.equal(typeof state, 'object', 'ChooComponentCache: state should be type object') | ||
| assert.equal(typeof emit, 'function', 'ChooComponentCache: state should be type function') |
|
🎉 🎉 🎉 🎉 🎉 |
|
|
…state * upstream/master: 6.12.1 6.12.0 typings: add app to app.use cb (choojs#665) update dependencies (choojs#663) ci: Node 7 → Node 10 (choojs#661) Update bel and yo-yoify references in docs to nanohtml. (choojs#660) Build for UMD (choojs#617) 6.11.0 Use nanoassert in the browser (choojs#651) Switch to nanohtml (choojs#644) v6.11.0-preview1 choo components (choojs#639)
Replacement for #606. Thanks!