implement choo v5 API #425

yoshuawuyts · 2017-03-13T02:58:12Z

heaps of changes!

Mostly following what's in https://medium.com/@yoshuawuyts/choo-v5-bc775b007b5e, but we're not including the component thing, and we changed the router API slightly. All for more simplicity.

All the lib/ stuff should become their own modules, but yeah it's late and I can't be bothered. Also lol, what are tests even.

This version is gonna be released as v5.0.0-2 so y'all can have a play with it. Check out the example.js file for a full example. Probably wanna symlink in nanomorph until choojs/nanomorph#47 has been resolved.

Anyway, hope this is somewhat shows the direction I'm feeling for v5; feedback is heaps welcome!

edit: also yay for less API surface - we also dropped down to 4.3kB which is like neat. Huzzah

abradley2 · 2017-03-13T23:28:39Z

🎉
Awesome stuff Yosh! Great to see Choo getting even better!

Ok, so it looks like with the change to mutable state, we no longer have the view signature of

function (state, prev, send) {

since prev state is not a thing right? I had one big use case for prev

Sometimes I have a view that displays in response to a route like /posts/123

So on view create, I'd add an xhr request in response to the onload hook, which grabs the "post". However, if the user navigates from /posts/1 to /posts/2, the view isn't really created a second time. So onload doesn't work here. I need to grab my data when a piece of state has changed and the view has mounted.

To sum up what I was doing to solve this- in the render function I would compare the current and previous parts of the location.params to see if the user had navigated- to the same page, but with different parameters that would warrant me to fire my the functions i normally call in the onload hook.

Some prior art to solving this issue:

In vue, the vue router has a beforeRouteEnter and (added in a later version) a beforeRouteUpdate hook that will fire whenever the route is the same (and therfore won't switch components) but relevant route params have changed

So my first piece of feedback-
what do you think about an onupdate hook to complement onload?

aknuds1 · 2017-03-14T06:39:43Z

@abradley2 You might want to check out my choo-routehandler complementary route handling framework for Choo. It lets you define loading hooks for routes, which is what you need right? Bear in mind it's written for Choo v4 though...

cc: @timwis

bcomnes · 2017-03-14T21:58:24Z

My only suggestion would be to include a nanocomponant in the example, demonstrating the isSameNode method.

timwis

Looks great! I left some questions in example.js. Beyond that, I share @abradley2's concern about being able to detect route changes, and I might suggest moving example.js to an example directory so that you can have a distinct package.json. This would remove things like todomvc-css from choo's package.json, and also let you show users how to run choo apps with npm start and npm run build scripts.

timwis · 2017-03-15T10:20:35Z

example.js

@@ -0,0 +1,353 @@
+var mutate = require('xtend/mutable')
+var expose = require('choo-expose')


What's this library? It's not on npm

Derp, yeah I should publish it, it's still just on my machine - 10 lines tho but yeah lol

timwis · 2017-03-15T10:21:16Z

example.js

+css('todomvc-app-css/index.css')
+
+if (module.parent) {
+  exports.todoStore = todoStore


Might this be overkill for an example app? Or is there some reason to include this?

Oh yeah this is for the tests to run

ooooh, right - funny, i did the same thing in an app i'm writing so that tests could run last week. maybe a comment saying "so tests can run"? lol, unless you think most folks wouldn't wonder about that.

good suggestion

timwis · 2017-03-15T10:21:59Z

example.js

+  app.route('/', mainView)
+  app.route('#active', mainView)
+  app.route('#completed', mainView)
+  app.mount('body')


Is there no app.start() anymore? index.js suggests it still exists.

It's still there, and should be in the README! - the reason for app.mount() is that I like writing my apps as:

<body> <header></header> <main></main> <footer></footer> </body>

app.mount() is to replace a DOM node. (e.g. document.body), app.start() is to append to a DOM node.

oh, interesting. I didn't realise it was an either/or. I assumed you had to mount and start. k thx

timwis · 2017-03-15T10:22:44Z

example.js

+      <footer class="info">
+        <p>Double-click to edit a todo</p>
+        <p>choo by <a href="https://yoshuawuyts.com/">Yoshua Wuyts</a></p>
+        <p>Created by <a href="http://shuheikagawa.com">Shuhei Kagawa</a></p>


FYI, at first read, I thought this person co-authored choo. My guess is that they wrote the original version of the choo todomvc app.

timwis · 2017-03-15T10:24:32Z

example.js

+}
+
+function todoStore () {
+  return function (state, emitter) {


At first read, this made me think models needed to return functions, which is a bit less..simple, but the readme suggests they don't need to. If the example is meant to illustrate the simplicity, perhaps it's better if this doesn't?

timwis · 2017-03-15T10:26:51Z

example.js

+
+function todoStore () {
+  return function (state, emitter) {
+    var localState = state.todos


What I'm gathering from this block of localState modifications is that the way you update state in choo v5 is by modifying the global state object, which is passed down to "models" (things that are used). That pattern is a bit unclear here because it looks like we're modifying a local variable, and not actually persisting that globally anywhere. In fact, because of the way JavaScript variables work by reference with objects, we are modifying a global variable, but you'd have to be familiar with that to pick up on what's going on here. My suggestion would be to make modifications to state.todos.active = [] etc. directly for simplicity's sake.

👍 fair point

Its a cool pattern though. Maybe we can note it in the handbook.

timwis · 2017-03-15T10:27:27Z

example.js

+      localState.idCounter = 0
+    }
+
+    emitter.on('DOMContentLoaded', function () {


Why would we want to wait until the DOM is finished rendering to register our listeners and such?

Kinda copied this pattern from choo 4's subscription API - the subs were only ever booted after the app was loaded. I guess this also prevents weird things from starting if you ever run the code in Node

Interesting. I mean, most folks put their <script> tag at the bottom of <body> these days anyway, so it's already being run after most of the DOM is loaded, but I imagine if you're really trying to optimize, you'd want your JS to run even before your fonts are finished loading. (Though if you're really trying to optimize, you'll probably use system fonts lol, but you get the point)

yoshuawuyts · 2017-03-15T12:07:31Z

@abradley2 The prev argument in choo 4 was intended for component-like systems. With the latest release of nanocomponent it kinda showed that the prev argument lead to more complex APIs, complicated internals and could leak if not used right.

We also removed our internal copy of the location stuff because keeping it in sync was trouble, and if you want access to the location there's window.location anyway. If you wanna listen for changes though, there's the emitter.on('pushState') event that has the new location. I think that might perhaps be what you're looking for (:

With nanocomponent re-rendering based on new arguments can be expressed as:

var nanocomponent = require('nanocomponent')
var html = require('html')

var myComponent = nanocomponent({
  render: function (state) {
    return html`<div>hi</div>`
  },
  onupdate: function (el, location) {
    console.log('new location was passed, mutate the DOM node!')
  }
})

module.exports = myView

function myView (state, emit) {
  return html`
    <section>
      ${myComponent(window.location)}
    </section>
  `
}

@bcomnes I'm thinking of including nanocomponent in the handbook, and try and keep the README minimal - only explain sibling reordering (tbi) and node caching (without using modules)

@timwis fair point about having an example dir; I'm mildly on the fence about it because I was trying to mimick the hypercore directory layout... but yeah maybs we should just move it to a dir

bcomnes · 2017-03-15T15:22:50Z

The handbook is good too.

finnp · 2017-03-15T23:52:24Z

index.js

-    }
+  function toString (location, state) {
+    state = state || {}
+    return router(location, state)


This should return router(location, state).toString(), right?

ah lol, yeah ✨

finnp · 2017-03-16T00:36:21Z

index.js

-    }
-    _frame(state, prev)
+  function route (route, handler) {
+    router.on(route, function (params) {


Should this be function (params, state, emit) ? If so nanorouter would need to be patched: choojs/nanorouter#1

nah it's ok because we wrap it in the next line, no need to pass all those extra args

How does toString then passes the state argument around? Alternatively it could overwrite the state I guess.

oh dang, you're right - yeah I was thinking the latter tbh. Given it's all sync it's safe

timwis · 2017-03-16T10:56:33Z

At first glance, that onupdate stuff looks even easier to use than prev. @abradley2 what do you think?

yoshuawuyts · 2017-03-17T02:34:05Z

Implemented all feedback!

bcomnes · 2017-03-19T04:10:55Z

Are the recent changes released as a beta?

bcomnes · 2017-03-19T21:09:20Z

README.md

-anything else which might satisfy this requirement is fair game.
-
-Generally for production builds you'll want to run:
+`choo` uses [nanomorph][nanomorph], which diffs real DOM nodes instead of


fix link edit: nm

bcomnes · 2017-03-19T23:10:38Z

I found a few issues around input elements. I opened issues at nanomnorph, but I'm thinking maybe they should live here.

choojs/nanomorph#50:

If you type into an input type=search element, that text is cleared on every render event, even if the text data is unbound to state in any way.

choojs/nanomorph#48:

If you try to bind an input=range element to state, it doesn't reflect state till you click on it, resize it, or directly set the value property on the dom node. UPDATE: A work around is wrapping input search types with nanocomponent. Not a solution but works.

I've not been able to find the source of the bug on either of these. Any ideas?

yoshuawuyts · 2017-03-20T10:43:42Z

@bcomnes yeah, to get the latest pre-release run:

$ npm i choo@next

yoshuawuyts · 2017-03-21T02:01:12Z

✨ ✨ ✨ thanks for all the feedback everyone! ✨ ✨ ✨

bcomnes · 2017-03-21T02:30:04Z

That was fun!

yoshuawuyts added 4 commits March 13, 2017 03:54

implement choo v5 API

f93b5f9

fix tests

e346edd

5.0.0-3

a13a62e

4kb now

b42bf35

yoshuawuyts added 4 commits March 14, 2017 02:49

bump nanomorph

19fbc6a

5.0.0-4

af53d27

fix missing deps stuff

65c20b2

5.0.0-5

159d638

timwis reviewed Mar 15, 2017

View reviewed changes

yoshuawuyts added 2 commits March 15, 2017 15:29

fix travis

69fd987

fixup! oops standard

04a3fe9

finnp reviewed Mar 15, 2017

View reviewed changes

finnp reviewed Mar 16, 2017

View reviewed changes

yoshuawuyts added 4 commits March 16, 2017 17:21

chooexpose

f46f1b9

fix state passing

113b338

fixup! update example

65981d8

fixup! incorporate feedback

9c1b0fe

yoshuawuyts mentioned this pull request Mar 19, 2017

choo v5 release checklist #429

Closed

9 tasks

bcomnes reviewed Mar 19, 2017

View reviewed changes

fixup! more docs

65e6105

yoshuawuyts deleted the v5 branch March 21, 2017 02:00

		@@ -0,0 +1,353 @@
		var mutate = require('xtend/mutable')
		var expose = require('choo-expose')

implement choo v5 API #425

implement choo v5 API #425

Conversation

yoshuawuyts commented Mar 13, 2017 • edited Loading

abradley2 commented Mar 13, 2017

aknuds1 commented Mar 14, 2017 • edited Loading

bcomnes commented Mar 14, 2017

timwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts commented Mar 15, 2017

bcomnes commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwis commented Mar 16, 2017 • edited Loading

yoshuawuyts commented Mar 17, 2017

bcomnes commented Mar 19, 2017

bcomnes Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

bcomnes commented Mar 19, 2017 • edited Loading

yoshuawuyts commented Mar 20, 2017

yoshuawuyts commented Mar 21, 2017

bcomnes commented Mar 21, 2017

yoshuawuyts commented Mar 13, 2017 •

edited

Loading

aknuds1 commented Mar 14, 2017 •

edited

Loading

timwis commented Mar 16, 2017 •

edited

Loading

bcomnes Mar 19, 2017 •

edited

Loading

bcomnes commented Mar 19, 2017 •

edited

Loading