Introduce plugin architecture. #124

jorgebucaran · 2017-03-02T17:10:46Z

Plugins are used like this:

app({
  ...,
  plugins: [PluginFoo, PluginBar, PluginBaz]
})

In a nutshell, if you know how to make an app with HyperApp you already know how to make a plugin.

A plugin is a function that exports an object with the same props as the object you pass into app(options) (except views, plugins don't export views at the moment, but you can listen to the onRender hook to alter this process, see Router).

For example, a plugin that adds a new subscription to your app:

const MyPlugin = app => ({
    subscriptions: [ _ => console.log("Hi, I'm a plugin") ],
})

A plugin function is called by HyperApp during your app's init step (before rendering a view). The app argument is the same options object you pass into app(options). Plugins don't need to modify this object, instead they return an object that is merged back into your app's options.

Since this is JavaScript, plugins can totally modify this object's properties, so they have both the power to bring joy and devastation to the world.

I'll be updating the docs later, once this ships.

Also on this branch:

Entire test spec rewritten.
New Router based in the new plugin architecture.
New and faster h.
- As a result, smaller app.js since we no longer need to handle bools/nulls in the vdom engine.

codecov · 2017-03-02T17:10:53Z

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #124   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines         170    174    +4     
=====================================
+ Hits          170    174    +4

Impacted Files	Coverage Δ
src/app.js	`100% <100%> (ø)`	✅
src/router.js	`100% <100%> (ø)`	✅
src/h.js	`100% <100%> (ø)`	✅
test/util.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 94ae5b4...f0503df. Read the comment docs.

jorgebucaran · 2017-03-02T17:17:01Z

@tunnckoCore @lukejacksonn Could you review this PR too? 🙏

lukejacksonn · 2017-03-02T17:21:24Z

Sure, I will take a look this afternoon! 👍

lukejacksonn · 2017-03-02T17:31:48Z

src/router.js

+		],
+		hooks: {
+			onRender: function (model, view) {
+				console.log(model.router.location)


console.log here 🔥

lukejacksonn · 2017-03-02T17:33:28Z

src/router.js

+			}
+		}
+
+		return routes["/"]


Were we going to be implementing the changes made in #113 to this PR?

Better after this is landed 👍.

tunnckoCore · 2017-03-02T17:48:37Z

Too much set in one commit.. but i'll try. In first glance i have some notes.

terkelg

Looks good to me!

tunnckoCore · 2017-03-02T17:52:04Z

src/app.js

-		onUpdate: Function.prototype,
-		onError: function (err) {
+	var onError = function (err) {
+		if (!hook("onError", function (fn) {


This is very strange to me.. Consider moving the callback function before the if?

var onerror = function (fn) { fn(err) } if (!hook("onError", onerror)) { throw err }

and in anyway it looks strange, but that's better

That's not going to work, because we pass onError to effects and subscriptions.

wat? no, no. I not meant that. It's no matter how the var is named. The point is to not be inside the if condition

var foobar = function (fn) { fn(err) } if (!hook("onError", foobar)) { throw err }

and that (as currently) are the same thing

if (!hook("onError", function (fn) { fn(err) })) { // collapsed in one line throw err }

Maybe I'm missing something, but I still think this is not going to work when sharing onError with multiple plugins. This is a minor thing, so please send me a PR after we land this one 👍.

With the new hook system this problem has gone away!

tunnckoCore · 2017-03-02T18:03:27Z

src/router.js

+					params[re.keys.shift()] = arguments[i]
+				}
+
+				match = function (model, actions) {


Move function out of the replace? Move it inside the outside if. It would be faster for sure. It may sounds like a super micro optimization, but not only that won't redefined that function on each found match, but it would looks a bit better and natural.

Nope. It isn't (looking at the latest commit) L54-56.

Simply make match = 1 and if there is no matches in that .replace() function match would be falsey value, otherwise will be 1.

path.replace(new RegExp(re.re, "g"), function () { for (var i = 1; i < arguments.length - 2; i++) { params[re.keys.shift()] = arguments[i] } match = 1 }) if (match) { return function (model, actions) { return routes[route](model, actions, params) } }

Hehe, done, locally. I haven't pushed anything 😄.

FlorianWendelborn · 2017-03-03T03:38:02Z

src/router.js

-            }
-        }
+			match = function (model, actions) {
+				return routes[route](model, actions, params)


Shouldn't params be in the model now?

This should actually also improve performance since match doesn't need to be executed on every render and instead just on every go. I'd not be surprised if this gives us -0.1 or more relative performance. (in addition to being less hacky and being easier to use)

Not sure I understand what you are saying 🤔.

Shouldn't params be in the model now?

Not sure about this. location is in the model not because it's the only way for the Router to control when the view is rendered without using a hack like passing a render function as we were doing before.

From #120:

model.router = { matched: '/some/route/:id', url: '/some/route/34593458', params: { id: 34593458 } }

I think it makes more sense and it should also be faster, since right now we call match every time the model is updated. That function looks quite expensive. I propose that go also tells setLocation the match() results so that it can be added to params.

This also has the advantage that actions may become easier since they can access the route params.

@dodekeract Ah, I think I see what you are saying now! It doesn't make sense to call match every time the view is rendered 👍.

I prefer:

params.id // or (model, actions, { id }) -> id

over

model.router.params.id

But, I was able to extract the match functionality outside onRender.

@jbucaran Your syntax proposal is compatible with mine. You can just call the render function with model, actions, model.router.params.

FlorianWendelborn · 2017-03-03T03:46:07Z

src/app.js

+		}
+
+		if (app.hooks) {
+			hooks.push(app.hooks)


Instead of pushing objects into an array I would iterate through the objects and construct this data-structure:

var hooks = { onRender: [f1, f2, f3], onError: [f4] }

This has the advantage that access is fast and easy. Basically it should be handled the same as subscriptions.

@dodekeract Can you provide some pseudocode?

var hooks = { onRender: [] } function addToHooks(hook, name) { hooks[name].push(hook) } function handlePluginHooks (hooks) { for (var name in hooks) { addToHooks(hooks[name], name) } }

Seems more verbose. Specially if more hooks are added in the future. I'll see if there are any performance gains! 👍

There will be performance gains. https://jsperf.com/for-in-or-for

1,400,835,640 vs 43,479 for me.

Are you talking about for-in used with arrays?

Right, yes. Hard to keep track of 10 conversations at the same time.

FlorianWendelborn · 2017-03-03T03:46:51Z

src/app.js

+	}
+
+	function hook(name, fn) {
+		for (var key in hooks) {


Don't use for in for array iteration. It's slow and potentially buggy.

#124 (comment)

FlorianWendelborn · 2017-03-03T03:48:12Z

src/app.js


-	domContentLoaded(function () {
-		root = options.root || document.body.appendChild(document.createElement("div"))
+	for (var key in plugins) {


Don't use for ... in ... for array iteration.

On the other hand for loop is more verbose and we'd need to check if it's empty or not with .length. I'll experiment with for and see how that affects size/performance. 👍

Size might be more, but for in is basically a hack when used on arrays.

Just out of curiosity how come for ... in is being preferred over forEach or map

@lukejacksonn for in is used to loop through props of an object. This can be used to traverse an array too, but it's a "hack". The best practice to traverse arrays is using a regular for loop or forEach.

I've changed them all to use a regular for loop, so say bye to for in! :)

FlorianWendelborn · 2017-03-03T03:48:36Z

src/app.js

-			subs[key](model, actions, hooks.onError)
+		render(model, view)
+
+		for (var key in subscriptions) {


for ... in

#124 (comment)

FlorianWendelborn · 2017-03-03T03:49:35Z

src/app.js

@@ -53,32 +52,72 @@ export default function (options) {

 			if (typeof action === "function") {
 				container[key] = function (data) {
-					hooks.onAction(name, data)
+					hook("onAction", function (fn) {


if you implement the data structure I suggested this would be

hooks.onAction.forEach(hook => hook(name, data))

I see, this sounds good. Let me see how I can do that.

FlorianWendelborn · 2017-03-03T03:52:42Z

src/index.js

@@ -1,5 +1,5 @@
 import h from "./h"
 import app from "./app"
-import router from "./router"
+import Router from "./router"


Why is this TitleCase? It's not a class.

It's not a class.

I don't care about that convention. I think it sucks to limit myself to a convention for a construct I rarely use in JavaScript anyway. I don't use Classes/Prototypes.

Router is more distinctive. It's not a component, like in React, but they also use Router, which is familiar.

@jbucaran the thing is that it implies that router has to be used like new Router().

I don't use new or this; as an extension, neither does HyperApp. So, why should Router imply that?

Because it's TitleCase. For someone familiar with javascript (and not that much with hyperapp's source-code) that usually means it has to be instantiated.

Plugins written in title case look better, just like Components.

app({ plugins: [ Router, Logger, TimeDebugger, FusionGenerator, FluxCapacitor ] })

Router could be a plugin but that doesn't mean it has to be. If it is then it negates the R vs r issue. The awkward thing about the router is that it is not needed for all cases, but in reality it is needed for a lot of cases and if it is not needed (they only have one view/route) then hyperapp already has that covered with very little redundant bytes. Good frontend routers are hard to come by. I think this..

model.router = { matched: '/some/route/:id', url: '/some/route/34593458', params: { id: 34593458 } }

with an ~~onrender~~ onRender hook, feels like a very robust approach. I'd say for the sakes of argument.. abstract the router out, use it as an example of a really useful plugin (npm i hyperapp-router -S is a good practice for developers to get into anyway if we are encouraging a plugin ecosystem) and let people call it what they want.

Thanks @lukejacksonn. Notice the correct name is onRender. Hooks are application level events so we use camelCase. DOM events are lowercase so we use oncreate, onupdate, onremove for lifecycle methods. Just for the record and in case someone finds this in the future.

For what it's worth, the convention in Hapi is to capitalize anything you 'require'. It makes it more clear which variables are from an external file, and I agree with @jbucaran that it looks nicer.

Good to know! Components are also capitalized in HyperApp, but they are not classes.

FlorianWendelborn

Other than the for ... in ... it's good.

Since we use gzip I don't think there is any meaningful file-size difference in using the proper array iteration. There is a huge performance difference though.

FlorianWendelborn · 2017-03-03T13:28:15Z

src/app.js

-		})) {
-			throw err
+	var onError = function (error) {
+		for (var hookKey in hooks.onError) {


for in 😢

FlorianWendelborn · 2017-03-03T13:28:34Z

src/app.js

-					hook("onAction", function (fn) {
-						fn(name, data)
-					})
+					for (var hookKey in hooks.onAction) {


FlorianWendelborn · 2017-03-03T13:28:37Z

src/app.js

-						hook("onUpdate", function (fn) {
-							fn(oldModel, model, data)
-						})
+						for (var hookKey in hooks.onUpdate) {


FlorianWendelborn · 2017-03-03T13:28:43Z

src/app.js

-		hook("onRender", function (fn) {
-			view = fn(model, view)
-		})
+		for (var i in hooks.onRender) {


@dodekeract Using for in here is the easiest way to avoid checking if onRender is undefined.

lukejacksonn · 2017-03-03T16:00:11Z

src/app.js


-	domContentLoaded(function () {
-		root = options.root || document.body.appendChild(document.createElement("div"))
+	for (var key in plugins) {


Just out of curiosity how come for ... in is being preferred over forEach or map

jorgebucaran · 2017-03-03T17:38:29Z

@dodekeract Thanks for the comments. I removed several instances of for..in in favor of a for loop, updated the hooks system as suggested and revamped the router.

jorgebucaran · 2017-03-03T17:40:08Z

Hi @tzellman @danigb! Could you take a loot at the tests? I rewrote the entire spec, but even with 100% coverage maybe there are still features untested or missing cases?

lukejacksonn · 2017-03-03T22:24:23Z

.editorconfig

-[*.{diff,md}]
-trim_trailing_whitespace = true
-
-[*.json]
 indent_style = space
 indent_size = 2


Introduce plugin architecture.

Jorge Bucaran added 2 commits March 3, 2017 01:56

Plugins, new tests, faster h().

ccf3cbd

Trim trim.

04ceecb

jorgebucaran added the enhancement New feature or request label Mar 2, 2017

jorgebucaran self-assigned this Mar 2, 2017

jorgebucaran requested review from FlorianWendelborn and terkelg March 2, 2017 17:10

lukejacksonn reviewed Mar 2, 2017

View reviewed changes

terkelg approved these changes Mar 2, 2017

View reviewed changes

jorgebucaran mentioned this pull request Mar 2, 2017

Performance and Laziness #75

Closed

tunnckoCore approved these changes Mar 2, 2017

View reviewed changes

Jorge Bucaran added 3 commits March 3, 2017 05:39

Tests/Coverage 💯

6404b60

Update default route tests.

84977a8

Move match and regexify outside of Router function.

3da2f33

FlorianWendelborn suggested changes Mar 3, 2017

View reviewed changes

Jorge Bucaran added 2 commits March 3, 2017 20:04

Organize hooks by type into arrays and run instead.

5b7600c

Rewrite router.

1b765da

jorgebucaran added this to the 0.7.0 milestone Mar 3, 2017

FlorianWendelborn approved these changes Mar 3, 2017

View reviewed changes

lukejacksonn approved these changes Mar 3, 2017

View reviewed changes

Jorge Bucaran added 2 commits March 4, 2017 02:34

Use spaces and indent of 2.

6dee98c

Prefer for loop over for..in in some places.

f0503df

lukejacksonn reviewed Mar 3, 2017

View reviewed changes

jorgebucaran merged commit f81a6b5 into master Mar 4, 2017

jorgebucaran deleted the plugin-architecture branch March 5, 2017 10:02

jorgebucaran added a commit that referenced this pull request Jan 7, 2018

Merge pull request #124 from hyperapp/plugin-architecture

8255da7

Introduce plugin architecture.

Introduce plugin architecture. #124

Introduce plugin architecture. #124

Conversation

jorgebucaran commented Mar 2, 2017 • edited Loading

codecov bot commented Mar 2, 2017 • edited Loading

Codecov Report

jorgebucaran commented Mar 2, 2017

lukejacksonn commented Mar 2, 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

tunnckoCore commented Mar 2, 2017

terkelg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunnckoCore Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

jorgebucaran Mar 3, 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

FlorianWendelborn Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianWendelborn Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

FlorianWendelborn Mar 3, 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

FlorianWendelborn Mar 3, 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

FlorianWendelborn Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

jorgebucaran Mar 3, 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

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 Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgebucaran Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

FlorianWendelborn Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

jorgebucaran Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

lukejacksonn Mar 3, 2017 • edited by jorgebucaran Loading

Choose a reason for hiding this comment

jorgebucaran Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgebucaran Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

FlorianWendelborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgebucaran commented Mar 2, 2017 •

edited

Loading

codecov bot commented Mar 2, 2017 •

edited

Loading

tunnckoCore Mar 3, 2017 •

edited

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

FlorianWendelborn Mar 3, 2017 •

edited

Loading

FlorianWendelborn Mar 3, 2017 •

edited

Loading

FlorianWendelborn Mar 3, 2017 •

edited

Loading

FlorianWendelborn Mar 3, 2017 •

edited

Loading

FlorianWendelborn Mar 3, 2017 •

edited

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

FlorianWendelborn Mar 3, 2017 •

edited

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

lukejacksonn Mar 3, 2017 •

edited by jorgebucaran

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

jorgebucaran Mar 3, 2017 •

edited

Loading

jorgebucaran commented Mar 3, 2017 •

edited

Loading

lukejacksonn Mar 3, 2017 •

edited

Loading