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

Make the router use components, but only accept view functions #2281

Closed
dead-claudia opened this issue Nov 4, 2018 · 4 comments
Closed

Make the router use components, but only accept view functions #2281

dead-claudia opened this issue Nov 4, 2018 · 4 comments
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Milestone

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Nov 4, 2018

Updates
  • Switch examples to ES6+
  • Change m(m.route) to m(m.route.init)
  • Fix a bunch of mistakes
  • Update to fit with current proposal in Simplify our component model #2295, simplify Media

This is part 4 of a broader proposal: #2278.

This is the third major step of how Mithril would become a lot more component-driven and less component-oriented. My goal here would be to further encourage component use over definition, by changing m.route to be a bunch of components that accept and use view methods rather than components, so it's easier to use and decompose as necessary. More specifically:

  1. I'd make it into a component accepting view functions rather than a factory accepting components. This would simplify the API greatly.
  2. I'd change m.route.link to be itself a component rather than an oncreate method. This would make it far more flexible and less of a general hack.
  3. I'd make m.route.prefix a simple property. It'd fix the issue of reading (which is sometimes useful), but it'd also simplify our internals.
  4. I'd add the concept of child routing and child routing contexts, for easier extensibility.
  5. I'd yank out most of the route resolver functionality, replacing it with something a little better suited for transient things like authorization checks and prompts.

This would continue to use global state, but it'd limit how much is actually used. The routing is partially static, partially dynamic, and this is by design. I wanted to have the flexibility of dynamic routing (React Router was a heavy influence), but while still keeping as much as possible of the simplicity and conciseness of Mithril's existing static router. (If you're lost about what the difference is, React Router's docs go over this in high detail.)

So here's how that'd look.

Global routes

The global route API would be substantially simplified and made a bit more consistent so it's easier to teach. Also, by making it component-driven, it also allows Meiosis users to leverage the same API, enjoying the benefits of its simplicity without having to use m.mount.

For the API:

  • This is how you'd define global routes.

    m(m.route.init, {
    	// This is the default route, if none match. What's displayed here is
    	// the default.
    	default: "/",
    
        // This is optionally invoked on every route on first render with that
        // route.
        onmatch: render => {
            // Here, you can do things like intercept to prompt for auth and
            // similar. The return value is awaited before the new route is
            // rendered, but after the URL is updated. You can also redirect
            // here, even synchronously. Note that `m.route.get()` is up to date
            // here, so that's why it's not a parameter.
            //
            // If you need to render a new tree while you wait, you can call
            // `render(vtree)` with the temporary tree instead of the view, like
            // for a temporary auth prompt.
            //
            // Also, I plan to support async iterators being returned here, so
            // they can `yield` those trees instead of calling `render(tree)`.
            // It's a trivial usability improvement, and once IE can be shed,
            // we can deprecate and remove the `render(vtree)` in a future major
            // update and just always require async iterators here to do it.
        },
    
        // This is where you define the routes.
        routes: {
            // The key format is identical to the existing API, but the value is
            // a function accepting the child route context, and returning a
            // vnode tree to render.
            "/foo": childRoute => {
                // ...
            },
            "/bar/:id": childRoute => {
                // ...
            },
            "/baz/:path...": childRoute => {
                // ...
            },
        },
    })

    I removed the async resolver aspect of it, but replaced it with something that makes transient checks like authorization and payment much easier to accomplish.

  • This is how you get and set the prefix. What's set here is the default value, but Mithril reads this as a property when generating and parsing the routes.

    m.route.prefix = "#!"
  • This is how you get the current route. This simply becomes a property.

    const route = m.route.current
  • This is how you get the current route params. This simply becomes a property.

    const params = m.route.params
  • This is how you set the current route. Returns a promise resolved once the route change has been completed. All options are optional.

    var promise = m.route.set("/bar/:id", {
        data: {id: 1},    // The data you want to interpolate the path with
        params: {q: ""},  // The data you want to append as query parameters
        replace: true,    // Whether to replace the current history state
        state: {id: 1},   // The state to associate with the history entry
        title: "A really pretty page" // The title of the history entry
    })
    
    // Default options:
    var promise = m.route.set("/bar/:id", {
        data: {},
        params: options.data,
        replace: false,
        state: options.data,
        title: document.title
    })
  • This is how you create links.

    m(m.route.link, {
        // The same options for `m.route.set(options)` are accepted here. In
        // fact, we just send the attributes object directly to `m.route.set`,
        // since they don't conflict.
        path, data, params, replace, state, title,
    
        // A custom tag or component to use for the link. The default tag is set
        // here.
        tag: "a",
    
        // Custom attributes for the component itself. In addition to this, the
        // following attributes are overwritten:
        // - `href` for the resolved path
        // - `onclick` for proper click handling (wraps existing `onclick` if
        //   necessary)
        attrs: {
            // ...
        },
    }, ...children)

If you wanted to, say, include some responsive routes, you could build the route object dynamically in the view:

// This is a direct port of their example's view
const invoiceView = Component => m(Layout, [
    // always show the nav
    m(InvoicesNav),
    m(Component),
])

m(Media, {query: PRETTY_SMALL, view: screenIsSmall => m(m.route.init, {
    routes: {
        "/invoices": () => screenIsSmall
        // small screen has no redirect
        ? invoiceView(Overview)
        // large screen does!
        : m.route.set("/invoices/dashboard"),
        "/invoices/dashboard": () => invoiceView(Dashboard),
        "/invoices/:id": () => invoiceView(Invoice),
    },
})})
In case you're wondering where that `Media` component came from...

It's a stripped down port of react-media.

// Okay, and this is why I like Mithril: it's not super complicated and full of
// ceremony to do crap like this.
function Media(vnode, old) {
    let queryList = vnode.state

    if (m.changed(queryList, vnode, prev, "query")) {
        let mediaQueryList = window.matchMedia(query)
        // Safari doesn't clean up the listener with `removeListener` when the
        // listener is already waiting in the event queue. The `mediaQueryList`
        // test is to make sure the listener is not called after we call
        // `removeListener`.
        queryList = {
            matches: () => mediaQueryList.matches,
            cleanup: () => {
                mediaQueryList.removeListener(updateMatches)
                mediaQueryList = undefined
            }
        }
        function updateMatches() { if (mediaQueryList) m.redraw() }
        mediaQueryList.addListener(updateMatches)
    }

    return {
        next: queryList,
        view: vnode.attrs.view(queryList.matches()),
        onremove: queryList.cleanup,
    }
}

Local routes

This is an optional part of the m.route redesign, meant for:

  • Easier scaling in medium-to-large apps using our router, since they can decompose.
  • Reduced bandwidth for larger apps if they choose to lazy-load their routes.
  • Those who would prefer to keep their child routes defined with their views for stylistic purposes.

It just helps keep the routing system composable, so people can move child routes into separate modules and recompose them at will. Also, in larger CRUD apps, this could do wonders in helping reduce code duplication.

This is passed as the sole argument for route methods, and all of the global m.route methods would be available on route contexts, just with all links and such relative to the context itself rather than the global prefix:

  • m.route.initchildRoute.init
  • m.route.getchildRoute.get
  • m.route.setchildRoute.set
  • m.route.linkchildRoute.link
  • m.route.prefixchildRoute.prefix

Most of this would just consist of using a factory(prefix), where prefix is the initial route.prefix, returning a routing instance. The implementation of the rest is easy: m.route = factory("#!") and childRoute = factory(inst.prefix + matchedBase).

Other questions

Since this fully decouples m.route from the rendering algorithm or the m.mount functionality as a direct dependency, we could pull this out of the core distribution, or at least consider creating a bundle with it excluded. I suggest we avoid it initially until I confirm if any meaningful size gains arise from this. (I have to redo the router anyways, and I plan to simplify it like I did with m.mount/m.redraw in #2280.)

@dead-claudia dead-claudia added Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Nov 4, 2018
@dead-claudia dead-claudia added this to the post-v2 milestone Nov 4, 2018
@dead-claudia dead-claudia added this to Under consideration in Feature requests/Suggestions via automation Nov 4, 2018
@project-bot project-bot bot added this to Needs triage in Triage/bugs Nov 4, 2018
@dead-claudia dead-claudia removed this from Needs triage in Triage/bugs Nov 4, 2018
@dead-claudia dead-claudia moved this from Under consideration to In discussion in Feature requests/Suggestions Nov 12, 2018
@highmountaintea
Copy link

I'd make it into a component accepting view functions rather than a factory accepting components. This would simplify the API greatly.

Conceptually this is great, but I am not sure whether this would work for the paradigm described below:

I have a sample book store, that has two routes:

  • /book-list: the list of books for sale
  • /book-detail/:id: detail view of a book

The user starts out on the book-list view, then clicks on a book to go into book-detail view, the transition is:
click on link -> change route to /book-detail/:id -> AJAX to load detail -> display book detail

Component provides an init hook to load data when user arrives at a route, function does not.

If route takes view function instead, here is the transition:
click on link -> AJAX to load detail into global state -> change route to /book-detail/:id -> display book detail

@dead-claudia
Copy link
Member Author

dead-claudia commented Nov 13, 2018

Edit: Forgot to wrap the component in a keyed element.

@highmountaintea

Your routes would end up looking something like this:

m(m.route, {
	"/book-list": () => m(BookList),
	"/book-detail/:id": () => m(m.keyed, m(BookDetail, {key: m.route.params.id})),
	// ...
})

For the two components here:

  • BookList is pretty obvious here, and you probably already have something like it.
  • BookDetail is still mostly the same thing, just you'd have to handle the async stuff yourself:
    • On init:
      • Fire AJAX to load detail into component state.
      • Render the loading screen (if necessary).
    • On remove:
      • Abort the AJAX call if you're still loading.
    • When the AJAX completes:
      • Render the newly retrieved book detail.

The transitions are still relatively similar, just you would need to use the component yourself rather than pass them directly to Mithril.

I'm passing the book ID via key here so if you change routes, the component gets recreated and you don't have to diff everything yourself.

Your `BookDetail` component would end up looking something like this, once you're all said and done:
function BookDetail(vnode) {
	var type = "pending"
	var abort, book, error
	m.request({
		url: `/api/book/${vnode.attrs.key}`
		config: xhr => { abort = () => xhr.abort() },
	}).then(
		b => { type = "ready"; book = b },
		e => { type = "error"; error = e },
	)
	return {
		onremove: abort,
		view: () =>
			type === "pending" ? m(Loading) :
			type === "error" ? alertError(error) :
			renderBook(book),
	}
}

If Async in #2282 gets added as an official standard library component, it'd instead become pretty trivial (it's why that proposal exists):

m(Async, {
	init: onAbort => m.request({
		url: `/api/book/${vnode.attrs.key}`
		config: xhr => onAbort(() => xhr.abort()),
	}),
	pending: () => m(Loading),
	ready: book => renderBook(book),
	error: error => alertError(error),
})

@highmountaintea
Copy link

Thank you for the detailed explanation, it helps me understand this new proposal.

In the example above

m(m.route, {
	"/book-list": () => m(BookList),
	"/book-detail/:id": () => m(BookDetail, {key: m.route.params.id}),
	// ...
})

I was under the impression that BookDetail is itself a render function. Your explanation shows me that it's actually a function that returns a component, so I have the opportunity to either initialize local state in that function, or specify the init hook that can generate the initial state. Hopefully this is helpful to others as well.

@dead-claudia
Copy link
Member Author

See this comment.

Feature requests/Suggestions automation moved this from In discussion to Completed/Declined Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Development

No branches or pull requests

2 participants