Refactor events, lifecycle, no emit return, 💯 thunks, cut >100 bytes #317

jorgebucaran · 2017-07-26T19:11:02Z

Work in progress.

codecov · 2017-07-26T20:15:42Z

Codecov Report

Merging #317 into master will increase coverage by 0.62%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #317      +/-   ##
========================================
+ Coverage   99.37%   100%   +0.62%     
========================================
  Files           2      2              
  Lines         160    146      -14     
  Branches       51     44       -7     
========================================
- Hits          159    146      -13     
+ Misses          1      0       -1

Impacted Files	Coverage Δ
src/app.js	`100% <100%> (+0.68%)`	⬆️
src/h.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69676ce...7d40065. Read the comment docs.

tunnckoCore · 2017-07-27T14:03:11Z

Simplify the merge function and enforce the use of a POJO
(plain old JavaScript object) as your state property. This
means primitive types are no longer valid states.

Do you remember, i was talking about that from the beginning? :)

Refactor thunks. Remove built-in promise support in favor of
using thunk composition.

Don't like that if i understand it correctly. Didn't see it in the diff. can you show short example?

As about changing one more time the name of the app's argument... 😆 Incoming isn't "props" in any sense.

jorgebucaran · 2017-07-27T14:05:05Z

Yup.

Around here. 👈

lukejacksonn · 2017-07-27T14:54:29Z

@charlike I wasn't sure on Remove built-in promise support in favor of
using thunk composition. wither but I came around to the idea once I saw the implementation and benefits it brings.. a summary (to reenforce my own understanding at least) IIRC:

Action: (s,a,d) => ({ user: d })
Thunk: (s,a,d) => update => fetch('/users').then(x => update({ user: x }))

Where update is a function that accepts a state fragment actionResult, merges that with the current state and calls render. Not dissimilar to setState in react.

@jbucaran @Swizz correct me if I am wrong!

SkaterDad · 2017-07-27T15:08:34Z

Yeah, calling it "thunk composition" is confusing. It does seem like a more powerful feature, though.

The built-in promise support really only handled the "happy path", right? With these thunks you can more easily do state updates from Promise errors, too?

Also a fan of enforcing state to be an object 👍

jorgebucaran · 2017-07-27T15:21:51Z

@SkaterDad @lukejacksonn That works, but the beauty is that if you want "native" promise support like we had, you can still totally have it, not shown in your example, but in the following:

events: {
  resolve(state, actions, result) {
    return result && typeof result.then === "function" 
      ? update => result.then(update)
      : result
  }
}

Certainly, you'll want to wrap that into a mixin, or better, group it with other things like support for observables or generators (I don't know) into a nice preset.

Swizz · 2017-07-27T15:29:25Z

Using thunk basically allow you to operate an update of the state, whenever you want/need.
The given update function is no more than the internal one we use to trigger an update of the state.

For a sync action returning a partial state, the internal code will be the following :

const result = action(...)
update(result)

But, for an action returning a function, the internal code will be the following :

const result = action(...)
result(update)

Now, you understand that we are giving to the action result the internal update function.

By this way, it will be available to you to resolve to an update of the state whatever/whenever you want.

(state, actions) => update => PROMISE.then(x => update(x))

(state, actions) => update => OBSERVABLE.subscribe({ next: x => update(x) })

(state, actions) => update => CALLBACK_FN(x => update(x))

jorgebucaran · 2017-07-27T15:34:23Z

For the record, in case anyone is now confused. You may not need to use thunks.

Other than promises, whatever you were doing before will still work. For promises you'll have to use the following pattern:

myAction(state,actions,data) => update => 
  fetch('/users').then(user => update({ user }))

Or if you do like I showed in this example, then you can do this instead:

myAction(state,actions,data) => fetch('/users').then(user => ({ user }))

and that'd work too.

tunnckoCore · 2017-07-27T17:07:38Z

@jbucaran oh yea, examples clears things. It now reminds me the freactal which i really loved and thought for something like it, even before its release. :) So yea, i'm agree, gives power and flexibility.

Based on that i prototyped some ideas about that signatures of effects, reducers, render and hooks to be the same.

foo({
  // `state` here would be empty object or undefined (not sure),
  // since they should not have access to it
  effects: {
    bar: ({ actions }, arg1, arg2) => {}
  },
  // forced synchronous actions
  reducers: {
    qux: ({ state, actions }, arg1, arg2) => {}
  },
  // here actions and state are actually "props",
  // so probably easy mocking. When stateless components
  // the `state` will be empty object, for no errors
  render: ({ state, actions, ...props }) => {},
  hooks: {
    init: ({ state, actions }) => {},
    error: ({ state, actions }, err) => {}
  }
})

I don't know if i'm the only one that i see one pretty fantastic ability of the destruction: allows enforcing conventions.

Anyway, above is just a concept of what i like.

Swizz

Quick review but this is some things that startled me by reading the diff.

Swizz · 2017-07-28T08:02:59Z

docs/actions.md

@@ -1,182 +1,117 @@
 # Actions

-Use [actions](/docs/api.md#actions) to update the state.
+Actions are functions which take the current [state](/docs/state.md) and return a partial state or a [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a partial state. Actions represent the intent to manipulate the state tree.


So is promise resolving will be part of the core or wont ?

The answer is "won't", but i believe it isn't the correct question. This description isn't very clear, because it don't mention that you may return a function which gets one argument called update which may/should be used to update the state, and also that update function returns the merged new object.

Yeah, haven't updated it yet.

Actions represent the intent to manipulate the state tree.

I still disagree for apps that are more complex than a hello world. I very often use them to e.g. communicate via socket.io/postMessage/http/etc..

@dodekeract I'll mention that actions have others uses too.

Swizz · 2017-07-28T08:06:28Z

docs/api.md

@@ -95,11 +97,20 @@ See also [Actions](/docs/actions.md).

 #### ActionResult

-A partial state or [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a partial state.
+A partial state, [thunk](#thunk) or [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a partial state.


Now ActionResult can be whatever you want as the resolve event allows you to transform it.
Or you are telling about ActionResult is the only thing the core will understand do trigger an update of the state ?

Swizz · 2017-07-28T08:08:04Z

docs/events.md

@@ -85,13 +95,17 @@ app({
 The `emit` function is available as the return value of the [`app`](/docs/api.md#app) function call itself.


Not the truth at all, isnt it ?

Hence wip 😆

Swizz · 2017-07-28T08:09:21Z

docs/mixins.md

@@ -43,7 +44,7 @@ const ActionPerformance = (ignore = [], cache = []) => emit => ({
    afterAction() {


Use resolve instead.

Swizz · 2017-07-28T08:12:50Z

src/app.js

-  repaint(emit("init"))
-
-  return emit
+  return emit("load")


Why not :

return emit("load", emit)

To return emit by default for non breaking change here. And emit will be given to load as data to permit to return emit even if you are returning something else.

loads(state, actions, emit) { return { actions, emit } }

Yes! Please! Awesome idea @Swizz !

jorgebucaran · 2017-07-31T17:24:17Z

For the first time 💯% coverage and 141B cut. 🔪

andyrj · 2017-08-01T04:29:40Z

this PR is amazing! ❤️ LGTM

lukejacksonn · 2017-08-01T20:01:20Z

@jbucaran I have only been keeping an eye on changes made to app.js but like @andyrj says it is looking good from here, some nice refactors and better naming of things 👍 💯

selfup · 2017-08-05T22:06:43Z

Wait why are we getting rid of emit? 🤔 I don't know if I like getting rid of such a powerful feature 😭

This will make it harder for people to trickle into existing apps.

What is the new alternative? Are we at least returning something?

Ok it seems that we are able to return whatever we want in load as @lukejacksonn mentioned in Slack 👍

FlorianWendelborn · 2017-08-05T22:13:58Z

@jbucaran Will we still support beforeAction and other events that aren't mentioned in your commit? I definitely need some place to catch actions being called. Otherwise, I don't see a way to port my debug mixin to the next version of hyperapp:

jorgebucaran · 2017-08-06T04:33:42Z

@dodekeract I definitely need some place to catch actions being called.

Sure, beforeAction → action. There is no afterAction though.

jorgebucaran · 2017-08-06T04:34:36Z

@selfup Sorry for the confusion, that commit message is a bit outdated, thanks for pointing it out.

We'll continue to return emit.

selfup · 2017-08-06T09:01:45Z

@jbucaran Ok thanks for clarifying! 😄

jamen

Read through all the doc changes, I love it! Just a few nitpicks, and question about source.

jamen · 2017-08-06T18:51:01Z

src/app.js

-  var loaded = false
+  var root = props.root
+  var host = (root && root.parentNode) || document.body
+  var lock = false


could we just do var lock with no false? undefined will coerce to true, and it will just cycle between the booleans from there.

Let me see if that works.

jamen · 2017-08-06T19:02:59Z

docs/actions.md

-    }
+    Game: gameActions,
+    Score: scoreActions,
+    ...userActions


Another nitpick, but since when has it been convention to capitalize these? If you were to ask me it just seems odd: actions.Game.tick, actions.Score.add.

@jamen There's no convention, it's just my current personal preference (but I can change my mind). What do you think is best and how can we find a way to agree on this?

@jbucaran If there is some rationale behind it, it would be nice to include a note. Previously I just remember people writing them plainly: actions.game.tick, state.score.count, etc. and I had no major problems with this.

It does create a nice immediate distinction between your own stuff and stuff mixins bring in, but it is not really conveyed clearly in this example.

@dodekeract How do you feel about this point?

jamen · 2017-08-06T19:03:17Z

docs/actions.md

-    up: ({ count, maxCount }) => ({
-      count: count + (maxCount > count ? 1 : -maxCount)
-    })
+    setText: (state, actions, text) => ({ text })


Just a tiny nitpick how this is written. It feels inconsistent with everything below being written as:

setText(state, actions, text) { return { text } }

Does/can prettier have a preference? Or should they just coexist?

@jamen Will fix it right away.

@jamen Prettier doesn't have a preference here. It'll use either version, depending on what the original code was.

I'd be tempted to be real terse with this..

setText(state, actions, data) { return { text: data } }

zaceno

Just some minor things to fix in the docs. Overall I'm on board with the concepts, but there's too much changing to dig into all the details.

zaceno · 2017-08-07T20:30:00Z

docs/events.md


-### afterAction
+- Allow actions to return a [Promise](https://github.com/tc39/proposal-observable).


url is wrong. Points to observable docs instead of promise docs

zaceno · 2017-08-07T20:32:45Z

docs/lifecycle-events.md

-## oninsert
-
-The oninsert event is fired after the element is created and inserted into the DOM. Use this event to wrap third party libraries that require a reference to a DOM node, etc.
+The [oncreate](/docs/api.md#oncreate) event is fired after the element is created, but before it is attached to the DOM. Use it to start animations before an element is rendered, [adapt an external library](#adapting-an-external-library) that requires a reference to a DOM node, etc.


We're changing this right? It's going to be called after element has been attached to the dom, right?

@zaceno Correct. 👍

lukejacksonn

I just tried looking through all the doc changes to see if there was anything that stood out as incorrect or could be reworded. Nothing too major!

lukejacksonn · 2017-08-08T09:39:50Z

docs/CONTRIBUTING.md

+* **Be nice.**
+* **Be welcoming**: We strive to be a community that welcomes and supports people of **all backgrounds and identities**.
+* **Be considerate**: Remember that we're a world-wide community, so you might not be communicating in someone else's primary language.
+* **Be respectful**:  Not all of us will agree all the time, but disagreement is no excuse for poor behavior and poor manners.


📐 Double space here

lukejacksonn · 2017-08-08T09:52:39Z

docs/CONTRIBUTING.md

+
+- Feel free to create a new `test/*.test.js` file if none of the existing test files suits your test case.
+
+- Tests usually create an application using the [app](/docs/api.md#app) function and then check for `document.body.innerHTML` to match some expected string. The app call is async, so we often use the [patch](/docs/api.md#patch) event to detect when the view has been synthesized into an element.


Does this still hold true? Did we come to a conclusion on removing the patch event or not?

lukejacksonn · 2017-08-08T10:42:30Z

docs/actions.md

@@ -1,182 +1,129 @@
 # Actions

-Use [actions](/docs/api.md#actions) to update the state.
+Actions are functions which take the current [state](/docs/state.md) and return a partial state or a [thunk](#thunks). Actions represent the intent to manipulate the state tree.


Suggested rewording..

Actions are functions that take some payload data and return a partial state or a thunk. They are the only way to update the state tree.

Is this link to thunks correct? Or should it be Thunks

lukejacksonn · 2017-08-08T10:44:33Z

docs/actions.md

-    up: ({ count, maxCount }) => ({
-      count: count + (maxCount > count ? 1 : -maxCount)
-    })
+    setText: (state, actions, text) => ({ text })


I'd be tempted to be real terse with this..

setText(state, actions, data) { return { text: data } }

lukejacksonn · 2017-08-08T11:04:31Z

docs/actions.md

-  }
-})
-```
+Actions are often called as a result of user events triggered on the [view](/docs/view.md) or inside application [events](/docs/events.md).


triggered from a view or from inside of an application event.

lukejacksonn · 2017-08-08T11:08:31Z

docs/actions.md

-    }),
-    upLater: (state, actions) => {
-      setTimeout(actions.up, 1000)
+    defer(state, actions, data) {


how come we are using this syntax here.. and not

defer: (state, actions, data) => update => { ... update(newData) }

Ok, I see we are using this syntax everywhere..

lukejacksonn · 2017-08-08T11:10:44Z

docs/actions.md

  }
 })
 ```

-Actions can be written as [async functions](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function) too.
+Actions need not have a return value at all. You can use them to create side effects, call other actions, etc.


Perhaps worth mentioning explicitly that..
Actions that return nothing will not trigger a state update

lukejacksonn · 2017-08-08T11:15:29Z

docs/actions.md


-[Try it online](https://codepen.io/hyperapp/pen/Zygvbg?editors=0010)
+We iterate over your action keys, allowing for namespaced actions. Namespaces help you organize your actions into categories or domains.


Hyperapp iterates over action keys recursively during setup, this means that actions can be nested under developer defined namespaces.

lukejacksonn · 2017-08-08T11:18:34Z

docs/events.md


-### Interoperatiblity
+### Interop


Not convinced about this title 🤔

@lukejacksonn I thought it sounded okay. What else?

Not sure.. perhaps just Interoperatiblity

lukejacksonn · 2017-08-08T11:22:39Z

package.json

@@ -7,7 +7,10 @@
  "module": "src/index.js",
  "license": "MIT",
  "repository": "hyperapp/hyperapp",
-  "files": ["src", "dist"],
+  "files": [


What is this key used for (just out of curiosity)

It determines which files & folders get published to npm.
https://docs.npmjs.com/files/package.json#files

@SkaterDad thanks!

- Refactor thunks. Remove built-in promise support in favor of using thunk composition. - Simplify the merge function and enforce the use of a POJO (plain old JavaScript object) as your state property. This means primitive types are no longer valid states. - Make lifecycle more consistent. → load (→ render → action → resolve → update) load: Use it to initialize and "load" your app resources here. This event fires before the very first render and actions you call here will be scheduled for render appropriately. render: Use it to overwrite the view function to "render" your own view. action: Use it to log or save information about actions. resolve: Use it to modify how actions work and what types they are allowed to return. update: Use it to inspect / validate the previous state against the future (new) state. If you return a new state inside this event, it will be used instead to update the current state tree. - Now onupdate is now called every time we patch an element. The function receives the element and the old data. This is a good place to write logic in response to prop changes. - More contributing under /docs. - Rewrite CoC based on the open code of conduct: http://todogroup.org/opencodeofconduct. - Remove rollup.config.js file, until now used only to specify the uglify plugin and use uglify on the command line directly.

jorgebucaran · 2017-08-08T20:28:50Z

Thank you everyone for helping to review this PR! Finally merging! 🎉

* Rename mixin composition to presets. * Rewrite tests. * Docs, tests, and other chores. * Refactor app.js, make variable names more consistent. - Refactor thunks. Remove built-in promise support in favor of using thunk composition. - Simplify the merge function and enforce the use of a POJO (plain old JavaScript object) as your state property. This means primitive types are no longer a valid value for `state`. - Make lifecycle more consistent. → load (→ render → action → resolve → update) load: Use it to initialize and "load" your app resources here. This event fires before the very first render and actions you call here will be scheduled for render appropriately. render: Use it to overwrite the view function to "render" your own view. action: Use it to log or save information about actions. resolve: Use it to modify how actions work and what types they are allowed to return. update: Use it to inspect / validate the previous state against the future (new) state. If you return a new state inside this event, it will be used instead to update the current state tree. - oncreate is now called after the element is created and attached to the DOM. - onupdate is now called every time we patch an element. The function receives the element and the old data. This is a good place to write logic in response to prop changes. - Both oncreate and onupdate are now invoked after patch returns. - Rewrite CoC based on the open code of conduct: http://todogroup.org/opencodeofconduct. - Remove rollup.config.js file, until now used only to specify the uglify plugin and use uglify on the command line directly.

Jorge Bucaran added 5 commits July 26, 2017 00:23

Refactor variable name.

39a61af

Rename mixin composition to presets.

6852548

Use f => f() to mock rAF instead of setTimeout.

ecfca42

Cosmetic: refactor code style in tests.

28cc508

Docs, tests, and other chores.

0257e80

jorgebucaran requested review from Swizz, lukejacksonn and zaceno July 26, 2017 19:11

Sync with master.

2ee8790

jorgebucaran force-pushed the refactor/events branch from 569bb5a to 2ee8790 Compare July 26, 2017 20:15

Swizz suggested changes Jul 28, 2017

View reviewed changes

jorgebucaran force-pushed the refactor/events branch from 473c819 to 3f7321d Compare July 31, 2017 16:53

jorgebucaran force-pushed the refactor/events branch from 3f7321d to e6be7a4 Compare July 31, 2017 19:11

andyrj mentioned this pull request Aug 1, 2017

Re-purpose / reuse root for SSR hydration / prerendering. #319

Closed

tunnckoCore approved these changes Aug 1, 2017

View reviewed changes

jorgebucaran force-pushed the refactor/events branch 2 times, most recently from 409a807 to a4c0e01 Compare August 3, 2017 12:48

jorgebucaran mentioned this pull request Aug 3, 2017

Fix issue where two structures that differ only on a value attribute are not updated correctly #321

Merged

jorgebucaran force-pushed the refactor/events branch 3 times, most recently from aa35a78 to 6b18681 Compare August 5, 2017 11:51

jamen suggested changes Aug 6, 2017

View reviewed changes

jorgebucaran mentioned this pull request Aug 6, 2017

Question about onremove #112

Closed

zaceno reviewed Aug 7, 2017

View reviewed changes

lukejacksonn suggested changes Aug 8, 2017

View reviewed changes

jorgebucaran force-pushed the refactor/events branch 4 times, most recently from e96c330 to 97f67fb Compare August 8, 2017 20:13

jorgebucaran force-pushed the refactor/events branch from 97f67fb to 9431cdc Compare August 8, 2017 20:20

Merge branch 'master' into refactor/events

7d40065

jorgebucaran merged commit 6ce6b93 into master Aug 8, 2017

jorgebucaran deleted the refactor/events branch August 8, 2017 20:28

This was referenced Aug 8, 2017

Add Typing (DTS file) #311

Merged

Support xlink:ref attribute #211

Closed

Crashes Without View #318

Closed

This was referenced Aug 11, 2017

load or loaded #324

Closed

Save a few bytes by shortening willRender #328

Closed

Swizz mentioned this pull request Jan 15, 2018

Better async actions support #558

Closed

		@@ -85,13 +95,17 @@ app({
		The `emit` function is available as the return value of the [`app`](/docs/api.md#app) function call itself.

		@@ -43,7 +44,7 @@ const ActionPerformance = (ignore = [], cache = []) => emit => ({
		afterAction() {


		### afterAction
		- Allow actions to return a [Promise](https://github.com/tc39/proposal-observable).


		- Feel free to create a new `test/*.test.js` file if none of the existing test files suits your test case.

		- Tests usually create an application using the [app](/docs/api.md#app) function and then check for `document.body.innerHTML` to match some expected string. The app call is async, so we often use the [patch](/docs/api.md#patch) event to detect when the view has been synthesized into an element.


		[Try it online](https://codepen.io/hyperapp/pen/Zygvbg?editors=0010)
		We iterate over your action keys, allowing for namespaced actions. Namespaces help you organize your actions into categories or domains.

Refactor events, lifecycle, no emit return, 💯 thunks, cut >100 bytes #317

Refactor events, lifecycle, no emit return, 💯 thunks, cut >100 bytes #317

Conversation

jorgebucaran commented Jul 26, 2017

codecov bot commented Jul 26, 2017 • edited Loading

Codecov Report

tunnckoCore commented Jul 27, 2017

jorgebucaran commented Jul 27, 2017

lukejacksonn commented Jul 27, 2017

SkaterDad commented Jul 27, 2017

jorgebucaran commented Jul 27, 2017

Swizz commented Jul 27, 2017

jorgebucaran commented Jul 27, 2017 • edited Loading

tunnckoCore commented Jul 27, 2017

Swizz 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

jorgebucaran commented Jul 31, 2017 • edited Loading

andyrj commented Aug 1, 2017

lukejacksonn commented Aug 1, 2017

selfup commented Aug 5, 2017 • edited Loading

FlorianWendelborn commented Aug 5, 2017 • edited Loading

jorgebucaran commented Aug 6, 2017

jorgebucaran commented Aug 6, 2017 • edited Loading

selfup commented Aug 6, 2017

jamen 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

zaceno 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

lukejacksonn 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

lukejacksonn Aug 8, 2017 • edited Loading

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

jorgebucaran commented Aug 8, 2017

codecov bot commented Jul 26, 2017 •

edited

Loading

jorgebucaran commented Jul 27, 2017 •

edited

Loading

jorgebucaran commented Jul 31, 2017 •

edited

Loading

selfup commented Aug 5, 2017 •

edited

Loading

FlorianWendelborn commented Aug 5, 2017 •

edited

Loading

jorgebucaran commented Aug 6, 2017 •

edited

Loading

lukejacksonn Aug 8, 2017 •

edited

Loading