choo components #639

yoshuawuyts · 2018-03-07T13:33:58Z

Replacement for #606. Thanks!

YerkoPalma · 2018-03-07T13:50:06Z

index.js

+  state.cache = renderComponent
+
+  function renderComponent (Component, id) {
+    assert.equal(typeof Component, 'function', 'choo-component-preview: Component should be type function')


I guess this is no longer a preview right?

goto-bus-stop · 2018-03-07T15:29:37Z

component/cache.js

+  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)


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.

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...

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...

The onload idea might be pretty difficult to implement in a generic way, though, I'm not sure.

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.

+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?

goto-bus-stop · 2018-03-07T15:30:53Z

example/components/footer/index.js

+module.exports = class Footer extends Component {
+  static identity () {
+    return 'footer'
+  }


the identity methods are no longer used, right? they are also defined in header and in info

yoshuawuyts · 2018-03-12T18:28:47Z

Some more eyes on this would be great if people have time! cc/ @choojs/trainspotters

ungoldman · 2018-03-12T21:05:51Z

Not to derail but would this make more sense as a middleware rather than shipping with core?

jondashkyle · 2018-03-12T21:39:55Z

@ungoldman First-class support for components in the framework helps tremendously with the accessibility effort. Having both state and emit exposed without any prop juggling in userland being the most notable gain, personally. Introducing to core is consistent with Choo “the framework” (albeit a smol one.) :)

yoshuawuyts · 2018-03-13T12:05:47Z

@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.

goto-bus-stop

this lgtm but i would also like signoff from someone who was more involved in designing it

YerkoPalma · 2018-03-13T12:36:09Z

I'm ok with this, but I think docs should also be updated, to reflect the use of components.

yoshuawuyts · 2018-03-13T12:37:50Z

ping @tornqvist @bcomnes @toddself your input on this patch would be greatly appreciated 🙏

tornqvist · 2018-03-13T13:06:10Z

index.js

-  // properties that are part of the API
-  this.router = nanorouter()
-  this.emitter = nanobus('choo.emit')
-  this.emit = this.emitter.emit.bind(this.emitter)


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.

Oopts, that was totally accidental. Great catch!

Fixed & added regression tests ✨

tornqvist · 2018-03-13T13:41:35Z

component/cache.js

+  if (!el) {
+    var args = []
+    for (var i = 0, len = arguments.length; i < len; i++) {
+      args.push(arguments[i])


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) { … } }

Nice, done!

yoshuawuyts · 2018-03-29T11:58:52Z

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! 😁

bendik · 2018-03-30T14:59:50Z

component/cache.js

+  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')


emit should be type function 🎹

Flet · 2018-03-30T16:28:07Z

🎉 🎉 🎉 🎉 🎉

yoshuawuyts · 2018-03-30T16:49:28Z

v6.11.0-preview1

…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)

yoshuawuyts and others added 13 commits March 7, 2018 13:35

add components

e410c0c

example: componentize header

70e9f5e

fix choo SSR

ae880ec

example: componentize footer

6c3105d

example: fix footer

3318a94

update component cache asserts

ec622e3

componentize todos

5066115

reorder example dir

246befe

fix example tests

f96becd

fix dep check test

06fa2dc

Add garbage collection of unused components

2645a5f

Apply args when calling identity

4765b90

update to new component preview

181b3bb

yoshuawuyts mentioned this pull request Mar 7, 2018

Components #606

Closed

YerkoPalma reviewed Mar 7, 2018

View reviewed changes

goto-bus-stop reviewed Mar 7, 2018

View reviewed changes

update nanocomponent

2f86431

tornqvist mentioned this pull request Mar 8, 2018

Expose initial state in head choojs/bankai#446

Open

2 tasks

yoshuawuyts added 2 commits March 9, 2018 14:12

fix cache err name

b7395db

remove static methods from example

3651662

yoshuawuyts changed the title ~~[wip] choo components~~ choo components Mar 12, 2018

goto-bus-stop approved these changes Mar 13, 2018

View reviewed changes

tornqvist reviewed Mar 13, 2018

View reviewed changes

restore app.emit() function

1c8ccc8

yoshuawuyts force-pushed the choojs-components branch 2 times, most recently from bccc530 to c1f621d Compare March 13, 2018 13:33

public API tests

c1f621d

tornqvist reviewed Mar 13, 2018

View reviewed changes

yoshuawuyts and others added 2 commits March 13, 2018 17:01

offset component cache iteration by 2

b9b2a83

whitelist contents of component folder (#643)

050649a

AntonTrollback mentioned this pull request Mar 15, 2018

Remove tachyons in favor of only normalize.css choojs/create-choo-app#56

Open

tornqvist mentioned this pull request Mar 22, 2018

Immutable state on SSR (toString) #649

Merged

allow lru number arg

39fc241

bendik reviewed Mar 30, 2018

View reviewed changes

fix lint typo

8662b8c

yoshuawuyts merged commit bddcfbe into master Mar 30, 2018

yoshuawuyts deleted the choojs-components branch March 30, 2018 16:19

Uh oh!

choo components #639

choo components #639

Uh oh!

Conversation

yoshuawuyts commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tornqvist Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goto-bus-stop Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ungoldman commented Mar 12, 2018

Uh oh!

jondashkyle commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoshuawuyts commented Mar 13, 2018

Uh oh!

goto-bus-stop left a comment

Choose a reason for hiding this comment

Uh oh!

YerkoPalma commented Mar 13, 2018

Uh oh!

yoshuawuyts commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flet commented Mar 30, 2018

Uh oh!

yoshuawuyts commented Mar 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tornqvist Mar 13, 2018 •

edited

Loading

goto-bus-stop Mar 7, 2018 •

edited

Loading

yoshuawuyts commented Mar 12, 2018 •

edited

Loading

jondashkyle commented Mar 12, 2018 •

edited

Loading