Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hook to router #1095

Closed
s3ththompson opened this issue Jun 5, 2016 · 107 comments
Closed

Add hook to router #1095

s3ththompson opened this issue Jun 5, 2016 · 107 comments
Milestone

Comments

@s3ththompson
Copy link

A feature request for the Mithril rewrite:

I love the simplicity of the current router, but I would like to expose a hook that runs after a route has been resolved but before the associated component has been rendered. The feature would be useful for implementing a few common patterns:

  • / routes to a landing page if the user is logged out, but /dashboard if the user is logged in
  • /dashboard routes to a dashboard if user is logged in, but /login if user is logged out

There is lots of precedent for this type of hook in other routers. See, for example, onenter in react-router and the ability to extend router.execute in Backbone.

Current solution in Mithril
var Dashboard = {
  oninit: function(vnode) {
      if (!Auth.loggedIn()) {
        m.route.setPath("/login");
      }
  }
// *** view code ***
}

Unfortunately, since setPath asynchronously triggers a reroute only after the window.onpopstate event fires, the dashboard component above renders to the screen for a split second before the login page is loaded.

Propsed API Options

Add an onroute hook to components that runs before the component is rendered and has the option to redirect to another route without rendering the component?

Alternatively, if oninit returns false, prevent rendering of the component?

@dead-claudia
Copy link
Member

@tivac This doesn't sound like a bad idea, although IMHO it belongs on the router, not the component.

@s3ththompson
Copy link
Author

Yes, I think it could belong on the router as well--I have no strong preference on the API design, just trying to figure out how to do this synchronously.

@lhorie
Copy link
Member

lhorie commented Jun 6, 2016

I think this is doable with the core router API, but yeah, would be nice to have the state machinery happen under the hood and have a friendlier hook.

@s3ththompson
Copy link
Author

Please let me know if I can help with an API proposal and/or pull request.

@andi1984
Copy link

andi1984 commented Jun 7, 2016

@lhorie What do you mean by that? Had same issue. Was fixing it by conditionally load the normal route config or a login route config routing everything to our login component. Do you mean something like that? Curious to hear from you.

@lhorie
Copy link
Member

lhorie commented Jun 7, 2016

@s3ththompson proposals are welcome

@andi1984 this refers to the rewrite branch, which has a "core" router module (./router/router) that is really low level, and a wrapper module that exposes an API that is more similar to v0.2.x (./api/router)

@tinchoz49
Copy link

Hi guys! what do you think about a different approach using middlewares.

var auth = function(vnode, next){
    user.auth(function(err, currentUser){
        if (err) return m.route.setPath("/login");
        vnode.attrs.user = currentUser;
        next();
    });
}
var customMiddleware = function(vnode, next){
    next();
}
m.route.setMiddlewares([auth]); // global middlewares

m.route(document.body, "/", {
    "/": [customMiddleware, home],
    "/login": login,
    "/dashboard": [customMiddleware, dashboard],
});

@andi1984
Copy link

andi1984 commented Jun 7, 2016

@lhorie Don't want to put pressure on the project or anything else. I love Mithril for its simplicity, but would like to use this rewrite in production. Any plans when 1.0 is more or less ready to use in production?

@lhorie
Copy link
Member

lhorie commented Jun 7, 2016

@andi1984 time-wise, I'm not sure. #1090 is keeping track of current status

@tivac tivac mentioned this issue Jun 8, 2016
22 tasks
@s3ththompson
Copy link
Author

I suggest keeping the router very minimal and not expanding to the full-blown concept of middleware.

What if m.route accepted a routes object that could have components as values or functions that allow the developer to manually resolve components:

m.route(document.querySelector('#app'), '/', {
  '/': function (resolve) {
    resolve((Auth.loggedIn()) ? Dashboard : Home);
  },
  '/about': About,
});

An explicit resolve function also allows us to redirect to another route using setPath instead of resolving a component for the current route:

m.route(document.querySelector('#app'), '/', {
  '/': function (resolve) {
    resolve((Auth.loggedIn()) ? Dashboard : Home);
  },
  '/about': About,
  '/account': function(resolve) {
    if (!Auth.loggedIn()) {
      m.setPath('/login');
    } else {
      resolve(Account);
    }
  }
});

@dead-claudia dead-claudia added this to the Rewrite milestone Jun 18, 2016
@s3ththompson
Copy link
Author

Any other thoughts on the design of this?

@dead-claudia
Copy link
Member

dead-claudia commented Jun 22, 2016

@lhorie @tivac What do you all think of this?

@tivac
Copy link
Contributor

tivac commented Jul 1, 2016

I like the second proposal, but don't like the function-passing aspect.

Something more like this would be my preference.

m.route(document.body, "/", {
    // normal component as it works now in 1.x and 0.2.x
    "/" : Home,

    // function arg that is expected to synchronously return the component to use
    "/secret" : function() {
        return authed ? Secret : Login;
    },

    // Don't always have to return if you're dynamically changing route
    "/redirect" : function() {
        if(authed) {
            return Secret;
        }

        m.route.setPath("/");
    }
});

@s3ththompson I think that solves your problem (& honestly one of mine in anthracite as well), would that work for you as an API?

@dead-claudia
Copy link
Member

dead-claudia commented Jul 3, 2016

@tivac I'm concerned how that would mesh with #1113. Classes are typeof C === "function", which might cause issues.

@lhorie
Copy link
Member

lhorie commented Jul 6, 2016

Update: I was experimenting with various ideas and use cases. What I currently have is this:

m.route(document.body, "/", {
    "/foo/:bar" : {resolve: function(render) {
        if (!auth) m.route.setPath("/login")
        else render(function view(args) {
            return m(Layout, {body: m(Test, args)})
        })
    }}
})

resolve runs on every route change. view runs on every redraw.

The use cases that this pattern covers are:

  • redirect before render on arbitrary condition (e.g. line 3)
  • asynchronous component resolution / code splitting (via render callback)
  • component composition (e.g. m(Layout, ...) on line 5)
  • availability of route arguments (via args on line 4)
  • extensibility (object w/ resolve key on line 2)

So feature-wise, I think this covers everything I'd like it to cover except caching of asynchronously-resolved components. The downside is that it's not super simple to use.

I'm looking for suggestions to support those use cases with a simpler API

@dead-claudia
Copy link
Member

dead-claudia commented Jul 6, 2016

@lhorie Have you thought of making the router something like m.route.stream("/default", {"/path/:param": Component}), returning a stream?

The stream would emit {route, component, params, matched: true} objects on every route change, where route is the current route, component is the route's component, and params contains the route parameters as an object. In the event the route is invalid, you emit {route, matched: false} instead. If the default doesn't match any of the routes, you can (and should) error out early, so it can be assumed everything is in order.

At the end, you could optionally do m.mount(elem, component), and you now have infinite flexibility. Oh, and the router is fully decoupled from the rest of the API. To initialize the first route, you should call router(router()), but you can add a router.update() method to make this look less obscure.

You could keep the current m.route API as sugar, and just build the stream primitive quite simply.

// The router implementation
function stream(defaultRoute, routes) {
    var router = m.prop()

    // router.set calls this automatically
    function update() {
        router(router())
    }
    // ...
    router.link = link
    router.prefix = prefix
    router.get = get
    router.set = set
    router.update = update
    return router
}

m.route = (function () {
    var router

    function route(elem, defaultRoute, routes) {
        router = stream(defaultRoute, routes)
        router.map(function (change) {
            m.mount(elem, m(change.component, change.params))
        })
        router.update()
    }

    route.stream = stream
    ;["get", "set", "link", "prefix", "update"].forEach(function (prop) {
        route[prop] = function () {
            if (router == null) throw new TypeError("Router not initialized yet")
            return router[prop].apply(router, arguments)
        }
    })
    return route
})()

@dead-claudia
Copy link
Member

As an added bonus, it's harder to screw up with not having the router initialized yet, because things won't work if you don't have them set up.

Obviously, the above doesn't include the dependency injection, but that would be pretty straightforward to do:

// api/router.js would look like this instead:
var coreRenderer = require("../render/render")
var coreRouter = require("../router/router")
var autoredraw = require("../api/autoredraw")

module.exports = function($window, renderer, pubsub) {
    var stream = coreRouter($window)
    var router

    function route(root, defaultRoute, routes) {
        if (arguments.length === 0) {
            if (router == null) throw new TypeError("Router not initialized yet")
            return router
        }

        router = stream(defaultRoute, routes)
        router.map(function (change) {
            if (change.matched) {
                renderer.render(elem, m(change.component, change.params))
            } else {
                router.set(path)
            }
        })
        router.update()
        autoredraw(root, renderer, pubsub, router.update)
    }

    route.stream = stream
    ;["get", "set", "link", "prefix", "update"].forEach(function (prop) {
        route[prop] = function () {
            if (router == null) throw new TypeError("Router not initialized yet")
            return router[prop].apply(router, arguments)
        }
    })
    return route
}

And, as another bonus, the internal API almost exactly mirrors the external one, so if you need the extra flexibility, there's minimal impact on your app beyond possibly an extra import when you need to switch.

@lhorie
Copy link
Member

lhorie commented Jul 7, 2016

@isiahmeadows the core router is already decoupled from everything else. The aim of the public router API is to be easy to use for the 99% most common use case. I think if the user needed to manually call m.mount or m.render, it would detract from its usability.

@dead-claudia
Copy link
Member

dead-claudia commented Jul 7, 2016

@lhorie The primary API implementation of my idea already does that, so it won't affect the 99% use case (note the router.update() right before the autoredraw call). But by the time you need to dive into internals like that, I don't think that one extra call is going to be much of a problem, and if you have a complex loading process, you might need to defer the initial load.

@khades
Copy link

khades commented Jul 8, 2016

why not handling route events in component? "onRouteInit" "onRouteChange"?

@dead-claudia
Copy link
Member

@khades That wouldn't make sense IMO. Routing has little to do with the components underneath. Conceptually, routing has little to do with components, anyways, unless you say "render this component for this route".

@lhorie
Copy link
Member

lhorie commented Jul 11, 2016

update: I added experimental support for resolve and render hooks

Usage:

var MyLayout = {
  view: function(vnode) {
    return m(".layout", vnode.attrs.body)
  }
}
var MyComponent = {
  view: function() {return "hello"}
}

m.route(document.body, "/", {
  "/": {
    resolve: function(use, args, path, route) { //runs on route change
      use(MyComponent) //may be called asynchronously for code splitting
    },
    render: function(vnode) { //vnode is m(MyComponent, routerArgs) where MyComponent is the value passed to `use()` above
      return m(Layout, {body: vnode}) //runs on every redraw, like component `view` methods
    }
  },
  "/foo": {
    render: function() {
      return m(Layout, {body: MyComponent}) // `resolve` method is optional
    }
  },
  "/bar": {
    resolve: function(use) {
      use(MyComponent) // `render` method is optional too, so this renders `m(MyComponent, routeArgs)` without layout
    }
  }
})

Redirections can be done this way:

m.route(document.body, "/", {
  "/": {
    resolve: function(next) {
      if (!loggedIn) m.route.set("/login")
      else next() //calling `use` with a different name here, but basically, resolve to undefined...
    },
    render: function() {
      return m(Layout, {body: MyComponent}) //...and hardcode the component here to save some typing
    }
  },
  "/test": {
    resolve: function(use) {
      if (!loggedIn) m.route.set("/login")
      else use(MyComponent) //again, if no custom layout composition needed, you can omit `render`
    }
  },
}

@pygy
Copy link
Member

pygy commented Jul 11, 2016

For the docs, use/next might as well be called render as you already do internally.

@pygy
Copy link
Member

pygy commented Aug 4, 2016

@lhorie a nit, but for consistency, assuming the component hooks keep their current names, one may want to add on to the router hooks as well.
So:

{
  onresolve: function (render, args, path, route){render(Component)},
  onrender: function(vnode) {return vnode}
}

One potential source of confusion in calling the first argument of onresolve render as I suggested is its signature: It takes a component whereas m.render and onrender only take vnodes.

Of note, your snippets above contains two occurences of the same bug (in case you were tempted to copy/paste them to the future docs):

    render: function() {
      return m(Layout, {body: MyComponent}) 
    }

MyComponent should read m(MyComponent).

Also, in both cases the parametrization of the component with args is lost since args can't be reached in onrender. This means that onresolve is needed if one wants to access the parameters. Not a big deal as long as it is documented properly.

@lhorie
Copy link
Member

lhorie commented Aug 12, 2016

@pygy if we're going to follow the onfoo convention, the method names need to be something else. resolve and render are what they do when some event happens

@orbitbot
Copy link
Member

Change the signature of what resolve expects, so you can do resolve(m('.layout', Foo ? Bar : Baz)).

The linked refactor looks cleaner, similar changes to that would be:

module.exports = function($window, mount) {
    var router = coreRouter($window)
    var currentComponent, currentArgs, currentPath
    var RouteComponent = {
        onbeforeupdate: function() {
            var livePath = router.getPath()
            if (livePath !== currentPath) {
                route.setPath(livePath)
                return false
            }
        },
        view: function() {
            return Vnode(currentComponent, null, currentArgs, undefined, undefined, undefined)
        }
    }
    var globalId
    var route = function(root, defaultRoute, routes) {
        currentComponent = "div"
        currentArgs = null

        mount(root, RouteComponent)

        router.defineRoutes(routes, function(payload, args, path, route) {
            var id = globalId = {}
            var resolved = false
            function resolve (component) {
                if (id !== globalId || resolved) return
                resolved = true
                currentComponent = component
                currentArgs = args
                currentPath = path
                root.redraw(true)
            }
            if (typeof payload === "function") {
                var result = payload.call(resolve, args, path, route)
                if (result) resolve(result)
            } else {
                resolve(payload)
            }
        }, function() {
            router.setPath(defaultRoute, null, {replace: true})
        })
    }

Remove the resolved check (might be dangerous?) and you can have easy placeholder elements while you're waiting for your async module to load... If the methods are renamed from view to something else then a source of confusion is definitely removed, but I think you could perhaps survive with only object vs function separation as an arguably nicer API. To be honest, I'd need to use either/both for a while to figure out what feels better.

@gilbert
Copy link
Contributor

gilbert commented Aug 24, 2016

Which async module loaders are we talking about? It might very well be possible to use async modules without any need to build resolve into mithril.

@lhorie
Copy link
Member

lhorie commented Aug 24, 2016

@orbitbot onmatch and render have fairly different semantics. I did try to merge the two but it ends up becoming fairly ugly to support all the use cases.

The main purpose of render that onmatch/resolve don't address is to recompute a view with new route parameters (for example when going from /grid?sort=asc to /grid?sort=desc). If resolve expected a vnode, then onmatch would need to be called on every redraw. That doesn't work because resolve can be called asynchronously, and then I'd need to pass some sort of flag to indicate a first-time resolve vs a redraw (or I would have to resolve a render-like function - at which point it might as well be a separate method)

@lhorie
Copy link
Member

lhorie commented Aug 24, 2016

@mindeavor In the docs, I have a webpack example, but presumably anything that can do code splitting would work

@pygy
Copy link
Member

pygy commented Aug 24, 2016

@lhorie The wrapper scenario is also made easier. I don't understand what RouteResolver.render adds to component.view in the /grid?sort=asc to /grid?sort=desc scenario... Aren't these passed as args/attrs anyway?

@gilbert
Copy link
Contributor

gilbert commented Aug 24, 2016

Ok, so here's a simple & easy way of doing async modules without explicit support from mithril. All you need is a mithril-friendly async load wrapper.

Let's call it myRequire. First, an example of using it:

m.route(document.getElementById('app'), '/', {
  '/': {
    view: function (vnode) {
      var Home = myRequire('Home.js')
      return Home || m('p', "Loading...")
    }
  }
})

And that's it! Here is the definition:

var loaded = {}

function load (moduleName) {
  if ( loaded[moduleName] ) {
    return loaded[moduleName]
  }
  else if ( loaded[moduleName] === undefined ) {
    // Load asynchronously
    someAsyncModuleLoader(moduleName, function (module) {
      loaded[moduleName] = module
      m.redraw()
    })
    loaded[moduleName] = false
  }
  else {
    // Async loading is currently in progress.
    // Do nothing.
  }
}

@lhorie
Copy link
Member

lhorie commented Aug 24, 2016

@pygy re: render vs view, render has diff semantics, view is wrapped in component semantics

routeResolver = {render: () => m(Layout, m(Foo))} // component structure is Layout > Foo
component = {view: () => m(Layout, m(Foo))} // component structure is AnonymousComponent > Layout > Foo

In the example above, if all your routes use RouteResolvers, then Layout isn't recreated from scratch. If you use components, then since the top level component is different for each route, the Layout gets recreated from scratch on route changes

@mindeavor onmatch is also there for pre-render redirects (i.e. calling m.route.set before the first render pass, thus avoiding flickering, potential null ref, unwanted lifecycle method calls and other issues)

@pygy
Copy link
Member

pygy commented Aug 24, 2016

I know the general semantic differences (I've re-implemented api/router.js twice already, tests all green), I don't understand why they matter in the specific case of sorting based on args change...

@gilbert
Copy link
Contributor

gilbert commented Aug 24, 2016

@lhorie Yes, and I think pre-render redirects are great :) I'm trying to remove the responsibility of choosing which vdom to render from onmatch.

@lhorie
Copy link
Member

lhorie commented Aug 24, 2016

@pygy for route args, render and view have the same behavior (but they can't be collapsed into one thing because of the difference I pointed out above). I know you're more familiar w/ the code, I'm just repeating for the benefit of others who may not be as familiar w/ the various use cases

@mindeavor you don't necessarily need to resolve to anything:

withFullSignature = {
  onmatch: function(vnode, resolve) {
    if (loggedIn) resolve() //resolve to nothing
    else m.route.set("/login")
  },
  render: function() {return m(Foo)} //hard code it here
}

@gilbert
Copy link
Contributor

gilbert commented Aug 24, 2016

@lhorie True,but I'm addressing many of the resolve(vnode) examples I'm seeing.

I think we need a new GitHub issue with a concrete list of requirements / desired use cases.

@lhorie
Copy link
Member

lhorie commented Aug 24, 2016

afaik what is currently in the repo addresses all the use cases I want, except #1180

other than that there's people giving various suggestions to try to conceptually simplify the API, but most of those suggestions don't address all the use cases

The use cases that I'd like to have are:

1 - some way to compose components with diff semantics (currently {render: () => m(Layout, m(Foo))})
2 - some way to do code splitting (currently {onmatch: (vnode, resolve) => require(["Foo.js"], resolve)}
3 - some way to do pre-render redirects (currently {onmatch: () => m.route.set("/")})
4 - some way to recreate from scratch on route reload (currently not supported without custom state machine hacks)
5 - route args should always be available for the above use cases
6 - all of the features above must be possible to use at the same time within a single route

@pygy
Copy link
Member

pygy commented Aug 24, 2016

I agree with you that it doesn't seem possible to simplify the API further.

Instead, Ill suggest an addition: a default, user-definable route.render that can be superseded on a per route basis by RouteResolver.render. That would simplify route declarations by removing boilerplate.

@lhorie
Copy link
Member

lhorie commented Aug 24, 2016

Can you give an example of usage?

@pygy
Copy link
Member

pygy commented Aug 24, 2016

route.render = function(vnode){return m('.defaultlayout', vnode)}

m.route(root, default, {
  '/foo': Component, // wrapped in the default layout
  '/bar': {onmatch: function(resolve){...}}, // ditto
  '/noDefaultLayout': {render: function(){return m('.bare')}} // just a div, no layout.
})

@barneycarroll
Copy link
Member

Thanks for the crucial feature list @lhorie. Here's a bin I believe ticks it all off in Mithril v0. You can turn faked random XHR errors on and off on the initial page - the dynamic component either defers draw til it can resolve the view with deferred resources, or redirects in the case of an error. The dynamic page opts to diff on route match. The error page mandates a full nuke of the DOM.

AFAICT hitting all of these in any given route with Mithril v1's clumsy distinction between RouteResolvers and Components is not possible to any useful degree. To wit:

  • You can't usefully combine routed 'code-splitting' & root-level DOM diffing on the same route endpoint in v1 since the former relies on resolving to a forcibly diffed root-level Component and the latter relies on resolving to a RouteResolver view. Making these features mutually exclusive, and to make that choice using this API, is of limited & unintuitive benefit IMO. Grokking that a v0 controller can pause or forego the usual lifecycle loop may involve explicitly invoking 1 of 3 APIs; using that same understanding and awareness of a 4th API to control diff behaviour may have been one API too many. But if you don't care, you don't care. If you want to make decisions about diffing, async state, etc... These APIs use reasonable language to describe those specifics. Mithril v1 isn't 'simpler' in the way it's rearranged this functionality - it's just brought in different terminology with less precision.
  • 'Code-splitting' is drastically impoverished in Mithril v1. The form of code-splitting most people writing SPAs will be familiar with involves separating a statically served front-end shell (app.js) from the data it populates - which is dynamically retrieved from web services. Mithril v0 had opinionated built-ins for this in the form of lifecycle-halting m.requests - but you could change that behaviour or avoid it entirely if you disagreed with that opinion. The classic example in Mithril v0 is that a route-level component halts view execution until it receives data which it then passes to the view. Mithril v1 turns this on its head by saying you can halt execution until it receives a glorified view structure which you can't pass anything into1.

  1. At the risk of boring everybody away from this thread forever I really must urge that we have a deep and honest think about this whole 'code-splitting' thing and its purported practical benefits, because I think there's some serious flaws in the value proposition here which are intrinsic to the confusion of concerns in RouteResolver / Component.

    Consider that managing async interactions with webservices is an established, ubiquitous, mundane, regular & essential part of any holistic SPA front-end framework. Consider that Mithril provides a complete batteries-included API for this. Consider that the application code for that task involves a pattern that involves calling a method, returning a wrapper for an async value, and binding it to a stateful object, which can later be interpolated to retrieve the value.

    Consider that splitting a front-end application's holistic structure into modules for piecemeal asynchronous invocation in a single runtime is a relatively recent phenomenon which is neither obvious in its application nor necessarily desirable, and requires architectural thinking beyond the scope of generic webservices and Mithril's immediate remit. Consider that Mithril provides none of the tooling necessary to implement code-splitting - only an API which can accept an object which could have been code split.

    resolve doesn't do anything to help with code-splitting - it's just a function which consumes an object. You can't implement async component loading and be under the illusion that it is doing anything useful with this regard, because you will have to have done all the significant work yourself to get there. And even if we're just aiming to solve some arbitrary aesthetic issue - that resolve( myComponent ) feels nicer than Object.assign( state, { myComponent } ) - then we must accept that our target audience is going to be disappointed when they discover that every other method of passing async values from one context to another necessitates assignment to an object. Lifecycle deferral - the ability to pause the Mithril run loop - is a feature, of course. But are the people making use of this feature really going to be impressed by the convenience of not having to write a placeholder div when they realise that this is the only place it's possible, and every async-data dependent view can't do this - and must explicitly write conditional views to accommodate pre-resolution state?.

@pygy
Copy link
Member

pygy commented Aug 24, 2016

@barneycarroll regarding your very last point, #1268 means that the UI is not responsive while the resolution is pending right now. If it is fixed (using either #1262 or the mount-based solution), then the previous component would remain live while the new one is loading. A spinner may be activated from the onmatch hook, no need to paint a placeholder while the delayed component is loading.

@lhorie
Copy link
Member

lhorie commented Aug 25, 2016

@barneycarroll I'm not really sure what you are trying to get at.

A large part of the rewrite effort in general is to modularize, which implies that each module has to have clear and localized semantics. m.redraw.strategy semantics and the start/endComputation semantics of v0.2's m.request are extremely disruptive; changing it risks breaking things in completely unrelated parts of the application. One can argue that this is powerful, but similar to C pointer arithmetic, it's also a huge foot gun, which hinders code scalability and requires an inordinate amount of discipline to manage.

The form of code-splitting most people writing SPAs will be familiar with involves separating a statically served front-end shell (app.js) from the data it populates

That's not what code splitting is. Code splitting is when you have an ERP system that serves a 1MB+ javascript bundle that contains code for several sub-applications, but you want to load each sub-application on demand to reduce initial page load time.

But again, saying onmatch/resolve is only about code splitting is missing the big picture. In a real life situation, the auth example relies on RouteResolver as well. Saying that those were possible in v0.2 ignores the disruptive nature of v0.2 semantics, the dichotomy of request-dependent redraws being more suitable for toy apps vs request-independent redraws being more suitable for production apps, the DOM-dirtying behavior of placeholders, etc etc.

Let me put things this way: given how obsessive I am about making a framework whose sell point is size, do you think I wouldn't have discarded RouteResolvers if I could solve all of its use cases elsewhere without relying back on semantics that were previously considered problematic?

@barneycarroll
Copy link
Member

@lhorie what I'm getting at is that under v1 the router API is more complicated and less powerful. It is possible to achieve the effect of some of the patterns in v0.2, but these are often mutually exclusive. The API design aims for a surface consistency with other APIs (vnode, attrs, component awareness) which give the illusion of a holistic top level controller, but because of the vagaries of the modularity dogma end up disappointing in their limitations.

It is possible to achieve all the stuff in the list given the full mithril.js package, but it would be incredibly convoluted compared to v0.2, where at least the APIs made holistic sense. You must be able to see that for authors to stomach the fact that they that either dynamically load a lifecycle-aware module or pass route-related attributes to the view is absolutely weird. The fact that this is a necessary trade off in order for another mythical demographic to be able to use Mithril's route engine without pulling in hyperscript or lifecycle modules is small comfort.

I went into great depths about the practical application of code splitting. I know exactly what you mean and addressed it with code samples acknowledging the practical reality and described deep shortcomings in that area too. In order to make the argument for authors who benefit from piecemeal Mithril modularity and dynamic module resolution, you must at least present a scenario whereby this would be the case. I can't see it.

The authentication scenario falls short at the same hurdle. Without building their own infrastructure to store the data relating to that authentication call, or the data that might populate any other arbitrary webservice request, authors are not going to thank you for the fact they had to manually compose their own modular Mithril build and write their own persistence layer to interact with component state, all for the benefit of being able to avoid the user having to download the redraw API. It doesn't add up in any practical scenario.

I appreciate the modularity requirement is a huge achievement, but personally I think this is appealing to the wrong crowd at the expense of engaged authors who want a decent API.

@barneycarroll
Copy link
Member

BTW this conversation is constant in that I'm illustrating all the use case scenarios and we end up talking at cross purposes because I'm doing the legwork of imagining, validating and explaining the use cases you're implying, but these end up being ignored because it's come down to an adversarial thing where everything I put up can be considered a straw man. It would really help if others wanted to put up examples of the current API dealing with Leo's matrix of features above.

@lhorie
Copy link
Member

lhorie commented Aug 25, 2016

the fact that they that either dynamically load a lifecycle-aware module or pass route-related attributes to the view

I don't understand what this means. Are you trying to say that this isn't possible:

var Layout = {view: function(vnode) {return m(".layout", vnode.children)}}
var Foo = {view: function() {return m("div", "hi from foo")}}

m.route(document.body, "/123", {
  "/:id": {
    onmatch: function(vnode, resolve) {
      resolve(Foo) // or require(["Foo.js"], resolve)
    },
    render: function(vnode) {
      return m(Layout, [
        m("div", "hi w/ id: " + vnode.attrs.id), // <- are you saying this is not populated because of the presence of an onmatch above?
        vnode,
      ])
    } 
  }
})

/* yields:
<div class="layout">
  <div>hi w/ id: 123</div>
  <div>hi from foo</div>
</div>
*/

all for the benefit of being able to avoid the user having to download the redraw API

I think you're missing my point when I mentioned modularity. Whether people use stuff piecemeal is completely irrelevant here. My point is that the semantics of the v0.2 APIs that you say are holistic are - in my personal opinion after using them in some real life projects - brittle.

For example, in v0.2 you can't just add a m.redraw() call anywhere. Depending on where you put, it can cause a null reference exception in a view in a completely different component. Why? Because you used m.request (!). We can't really mount much of an argument about how these semantics are useful in cases A or B if these same semantics cause crazy issues like that.

My understanding of what you've been saying so far is that RouteResolver sucks, and that its behavior can be achieved in v0.2 if we rely on semantics that are explicitly removed in v1 due to being problematic. Ok, that's true, but it's not actionable. Say we go ahead with your suggestion and remove RouteResolver. Then what?

@pygy
Copy link
Member

pygy commented Aug 25, 2016

@barneycarroll http://jsbin.com/jebusamibe/edit?js,console <= this may help...

Edit, BTW, vnode.tag being the route resolver is a bit weird, but it provides a window into the JSBin sandbox :-)

@lhorie
Copy link
Member

lhorie commented Aug 25, 2016

description of what that code is doing

There's a Layout component already loaded at page load and you want to render that plus some component that is not part of the initial bundle.

Let's look at a credible scenario: /issues/:issueId. As a Mithril v1 developer I am faced with a choice: do I resolve to to the issues component, or do I pass down the valuable information of the issueId to the view?

Why is this a choice? Both are happening, right?

@barneycarroll
Copy link
Member

@lhorie I'd like a description of what that code is doing and why it's useful. It's evidently possible, but it's not at all clear what it means, why I would want to do it, and what happens when Foo renders. Surely that last part is kind of pertinent? Either the view cares about route (as in the render function) or the view is determined by the route (resolve Foo), but the two are mutually exclusive. That doesn't make any sense from an application developer's perspective - it only makes sense from the perspective of justifying previous API decisions. Let's look at a credible scenario: /issues/:issueId. As a Mithril v1 developer I am faced with a choice: do I resolve to to the issues component, or do I pass down the valuable information of the issueId to the view? I'm honestly interested in your opinion on which is better, and why it should matter in application development. My gut feeling is that I'll be telling StackOverflow adopters that "that's the way it has to work, regardless of your concerns. but here's a cool hack from @pygy whereby you modify the DOM in onmatch". For my part, I think the distinction between these choices is obnoxious, and I can't hear a single voice that says it resolves any given problem. Mark my words: no-one will ever engineer a system whereby they request a unique component that combines data and component. That is THE anti-pattern the entire web movement has been working against this whole time. If that's an honest suggestion from Mithril, I may as well go back to PHP pages. Seriously, who benefits from this?

A huge amount of this debate relies on vague "yeah but it was a problem for some people I worked with". My contention is that coming up with new APIs does not in and of itself magic away those problems. Unless you can show me how you solve them!

Finally, you talk about m.request reference errors. What's the problem here? Have you solved it? Can you show me how things were a problem then and are no longer a problem now? Again, the nebulous justification seems completely disconnected from the purported solution.

@pygy
Copy link
Member

pygy commented Aug 25, 2016

Let's look at a credible scenario: /issues/:issueId. As a Mithril v1 developer I am faced with a choice: do I resolve to to the issues component, or do I pass down the valuable information of the issueId to the view?

You pass both, by default. Look again at the JSBin I posted above... In render() the component is in vnode.tag and the args in vnode.attrs.

And I'm not modifying anything in onmatch...

@lhorie
Copy link
Member

lhorie commented Aug 25, 2016

Not sure what happened with the order of the posts, but I already explained what the code does above.

I still don't understand what is "mutually exclusive". You say you want a "view to care about a route", but why? This is what that statement means:

var Issue = {
  view: function() {
    return m("div", "Issue id: " + m.route.param("id")) // this component would be more reusable if id was an argument
  }
}

I also don't understand what is obnoxious. Show me some v1 code and tell what it's supposed to do vs what it's doing. Or some code that should work but doesn't.

Unless you can show me how you solve them

Well, read the docs. Though their intricacies may be currently under-explained, all the use cases I care about and that have solutions are there. Again, if there are use cases that are not being satisfied by the current API, the proper way to address that is to open an issue describing actual vs expected behavior

If you have issues with the RouteResolver API, you're also welcome to open an issue with a proposal for an alternative that a) solves all the currently documented use cases, and b) highlights the problem you're trying to solve and how it solves it

@leeoniya
Copy link
Contributor

Personally, I find it rather awkward to mix routing concerns into the view layer. React does it out of necessity because Declarative All The Things! (tm), but it seems unnecessary to add this coupling into the core of more js-centric libs. It introduces a huge surface area for odd/unexpected behavior and bugs. In domvm, I chose to sidestep all these philosophical arguments and edge-case handling by letting the user define the coupling through a bit of router boilerplate that could easily be conventionalized as needed in app-space while keeping concerns fully decoupled in the core. Maybe something to think about.

/$0.02

@barneycarroll
Copy link
Member

@lhorie I realise I'm just complaining without offering solutions. The terminal problem with this conversation is that the defence of the current API is circular. It is the way it is because the rewrite has a mandate to separate concerns in a way that makes it impossible to bring those concerns back together for the router API, and once you've written it it's self justifying because whatever it does is whatever is possible. My parting message is that the routing docs address concerns which it presents as separate, but are intuitively holistic for authors, and these turn out to be mutually incompatible: you can't use the router's diffing strategy in combination with its asynchronous deferral. You do one or the other, or one then the other. This distinction, and the roundabout way it's handled, is of no benefit to authors. It may be the way the code 'has to work' for internal reasons, but the fact you can't mix and match with a single declarative object is frustrating.

I still contend that the idea 'routed component' that takes the best of both worlds - RouteResolver (diffing, deferred resolution) and Component (lifecyle) - which in layman's terms is essentially saying that route endpoint components can benefit from onmatch - is possible in v1: it simply requires a more open attitude to 'mixed concerns'. I'm going to pursue this as a plugin instead of just ranting :)

Consider this as my trying to bow out gracefully from the confrontational aspect of 'change the API'.

@leeoniya what you say about 'declarative all the things' is interesting (and BTW your thinking outside the box with DOMVM is really refreshing). Do you think we're being too obsessive about a view-focused paradigm in this conversation? And / or do you think we're being too drawn in to a desire for holistically static object APIs? How do you deal with concerns for deferred resource loading and diffing concerns with DOMVM?

@leeoniya
Copy link
Contributor

leeoniya commented Aug 26, 2016

@barneycarroll

Do you think we're being too obsessive about a view-focused paradigm in this conversation? And / or do you think we're being too drawn in to a desire for holistically static object APIs? How do you deal with concerns for deferred resource loading and diffing concerns with DOMVM?

For my personal taste, yes. I tend to design API/model first and then optionally introduce a view layer and router as consumers of this API, coupling them as needed. View-centric app construction (a la React) encourages everything to be stuffed into the view. If you keep your state dumb and immutable, where does your API live? Is it on the views and can only be invoked via the UI? Why should the router config live within the views? Why should a route definition reference a DOM element shudders? React is declarative all the way down, so we now have ever-growing declarative apis to accommodate all use cases when imperative code could solve things more obviously with less/no hacks. Despite writing domvm (a view layer), my preference is to have it simply be a UI for my domain model's APIs, which means it needs the ability to be maximally decoupled. The only surface area where the router and view interact is eg ["a", {href: router.href("users", 123)}, "User 123"]. Since domvm views do not have auto-redraw, you can invoke it at any time on any view (or subview). There are some DRY helpers in the router (like a didEnter(route) handler, so you can set up redraw / logging / whatever on any route entry).

I don't think that replicating in-view/in-vtree routing is a good idea, not in React, not in Mithril. It looks pretty and concise but introduces coupling and a large surface area for surprises, UB, hacking and bugs. If you guys can get it working well, great! But it's not my cup of tea. At the end of the day it's good to have multiple frameworks with different philosophies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests