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

[suggestion] Legacy router mode #2179

Closed
pygy opened this issue Jun 9, 2018 · 16 comments
Closed

[suggestion] Legacy router mode #2179

pygy opened this issue Jun 9, 2018 · 16 comments
Milestone

Comments

@pygy
Copy link
Member

pygy commented Jun 9, 2018

#1734 means that hash-based routing is broken in IE up to version 11.

To fix this universally, we must use onhashchange for hash-based routing unconditionally, but then we lose the state and option: replace in hash mode in browsers that support it.

Another solution would be to use pushState for all modes (with state and the possiblity to replace the current route), and have a one-time m.route.legacyHash() toggle that would switch to onhashchange-based routing (without any goodies).

What do you think?

Edit: legacy => legacyHash and then some

Edit2: code wise, this would amount to replacing

var supportsPushState = typeof $window.history.pushState === "function"

with

var usePushState = true
function legacyHash() {usePushState = false}

Once IE is forgotten, we can remove the legacy mode, apps that don't use it won't be affected, whereas the "universal" solution would entail another breaking change for everyone when we deprecate IE support.

@pygy
Copy link
Member Author

pygy commented Jun 9, 2018

Actually, as far as API goes, I'd rather add an extra, optional {onhashchange: true} param to m.route.prefix() so that the routing strategy is set in one API call.

@orbitbot
Copy link
Member

orbitbot commented Jun 9, 2018

Since there's a major bump coming, perhaps renaming to m.route.config would be appropriate in that case?

@pygy
Copy link
Member Author

pygy commented Jun 9, 2018

I'd rather not add unnecessary friction, even for v2.

Would you go for m.route.config({prefix, onhashchange})?

@barneycarroll
Copy link
Member

Can we say that this sits outside of the scope of 2? A line must be drawn.

@orbitbot
Copy link
Member

Just to be clear, I was thinking that if the naming makes more sense, the major version bump could be used to rename api methods... Not that it's totally necessary, I guess, for a single toggle there's a bunch of different alternatives. I don't immediately have a strong opinion on exactly how the API should be used TBH. An additional options parameter is probably also fine.

@pygy
Copy link
Member Author

pygy commented Jun 10, 2018

@barneycarroll I wish it were, but #1734 can't be fixed without a breaking change, I want it fixed ASAP and I don't want to do 3.0 just after 2.0.

This is a proposal for such a breaking change that is future-friendly.

In general, be it prefix() or config(), that method is cute but bad in terms of affordance (it must be called before m.route() and can't be called anywhere else).

It would make more sense to have something like this:

m.route({
  root: document.body,
  default: "/",
  routes: {...},
  config: {prefix, onhashchange}
})

@orbitbot
Copy link
Member

... Though arguably it might be cleaner with skipping the separate config subobject in that case, unless someone has a specific reason to keep it.

@pygy
Copy link
Member Author

pygy commented Jun 10, 2018

Indeed... Anyway, updating the test suite is going to be fun :-)

@pygy
Copy link
Member Author

pygy commented Jun 11, 2018

Here's what it would look like if we went for m.route({root, default, routes, prefix, onhashchange}):

https://github.com/pygy/mithril.js/compare/pygy:fix-1734-rebased-2018-6...pygy:fix-1734-revamped-api-2018-6?expand=1

@pygy
Copy link
Member Author

pygy commented Jun 26, 2018

@tivac any thoughts on the API bikeshed?

m.route.prefix(prefix, options?) seems like a fine compromise for both backwards and forward compatibility.

Alternatively, a codemod could also be used for migrating to `m.route({root, default, routes, prefix?, onhashchange?}).

While the latter is better in terms of affordance, m.route() and m.route.prefix() are not APIs that are touched often in a project's lifetime so having a cute if not optimal API isn't catastrophic...

@dead-claudia
Copy link
Member

dead-claudia commented Jul 8, 2018

@pygy I like that idea. And as it stands, I wouldn't say that is semver-major.

What are your thoughts on this instead? If semver-major is okay, this should be fine, too. As a proof of concept, I also put together a gist for this.

m.mount(elem, m.route.init({
    default: "/",
    prefix: "#!",
    resolve: (path, params) => ...,
    layout: (route, path, render) => ...,
    routes: {
        "/": params => ...,
        "/home": params => ...,
        "/view/:id": params => ...,
        // etc.
    },
}))
  • m.route.init({initial, routes, prefix?, layout?}) - Initialize the router, trashing existing state if necessary. Returns a component to mount via m.mount(elem, m.route.init(...)).

    • options.initial -> string - The initial route (required).
    • options.routes -> object - The usual route dictionary, but it can only be an object bag of "/route"(params) -> Children pairs (required).
    • options.prefix -> string - What's currently m.route.prefix.
    • options.resolve(path, params) -> [path, params, options?]? | Promise<[path, params, options?]?> receives a path with parameters and resolves it to either null/undefined if it should be the same or a [path, params, options?] pair if it should be different. options here are set options. (Default: a no-op)
      • This is how you implement redirects.
    • options.layout(render, path, params) -> Children - A method to specify the layout independent of routes to replace route resolvers' onmatch. (Default: return render())
      • render(params = params) is a wrapper for the resolved route function as specified in options.routes. If currently loading a new route, render itself is null instead.
      • path is the current path.
      • params is the current query parameters + template parameters, to replace m.route.params() and m.route.param(key).
      • If you want to know the route and query last rendered, you can just store the last rendered path/params/route for testing. (You normally shouldn't need to.)
  • m.route.set(path, {params?, replace?, state?, title?} = {}) - Set the current route, optionally with query parameters and history options.

    • If in a layout callback you call this before return/resolution, the resulting children are ignored and it triggers a redirect with the relevant options.
  • m(m.route.link, {to, tag = "a", attrs?, ...options}, ...children) - Create a vnode link to to with various m.route.set options, created using the tag name tag with various optional attrs (which may include other lifecycle methods).

    • Note that the href and onclick attributes are always overwritten, no matter what the original value is. This is for internal bookkeeping, and you won't get a working link otherwise.
    • This could technically be implemented in userland, but is provided internally for easier use.
  • Future: m.route.create(redraw, register) -> m.route - Create a new m.route instance with a different register callback and set of render hooks. This exists for testing and portability.

    • redraw - The redraw method you wish to use, defaulting to m.redraw.
    • register(callback, prefix) -> backend - Register a listener, and return a backend function for the router to use, optionally using the given prefix (inferred from options.prefix).
      • callback(path, state) - Send the parsed path and query object, with the given prefix. This should be called synchronously for the initial render.
      • backend.unregister() -> void - Unregister the callback, called when the router's onremove is called.
      • backend.set(path, options, redirecting) -> void - Set the path with the various options passed to m.route.set and whether it's updating per redirect. If you need to know what the current route is outside the component itself, you should use this.
    • When calling, if you wish to use the previously passed default for any of them, pass null/undefined instead of a value.

There are reasons behind certain design decisions I made for some of this:

  • Features:

    • The router is now much more minimal and intuitive. It just works, and it requires a lot less explaining.
    • Instead of worrying about getters, setters, and a giant router state machine where each state represents a component that's rendered, it's all just functions from a path and params to what you wish to display, with a little bit of backing persistent state.
    • The distinction between a route resolver and component (and all the issues it created) no longer exists.
    • The router could now be fully tree-shakable once we migrate to Rollup.
    • The router is now itself a component purely implementable in terms of userland APIs without the DOM. This alone opens several doors that wouldn't exist otherwise:
      • It would be usable with mithril-node-render without shim for things like server-side template rendering.
      • It's something we could trivially move out of core.
      • If m.route.create is added as per above, you could create a custom router that only deals with a small part of a page and has no concept of native history.
    • The router is now easy to abstract and extend, in a way that's fairly cohesive and very decoupled from Mithril's main architecture.
      • Excluding m.link, the only Mithril-specific things it depends on are a m.redraw implementation and the component structure (ignoring vnodes). It makes zero assumptions on the render result, and it'd be relatively easy to port to other frameworks.
      • This makes it a breeze to test.
    • We now have documented behavior on what happens when you call m.route.init more than once: it just resets the router state.
  • Reasons:

    • The reason I suggested making it all an object with m.route.init() replacing the base m.route() is because it's more self-documenting for users (compared to m.route(root, defaultRoute, routes)) and so it's easier for us to create, maintain, and export.
    • The reason I passed the route name only through one of the layout function parameters is because it's rarely useful outside of global values.
    • The reason I replaced route resolvers' render with route callbacks and layout callbacks is because most routes in my experience that aren't simple components are parameterized ones, and the router now doesn't care whether you use components or not.
    • The reason I replaced route resolvers' onmatch with layout callbacks is so I could centralize the decision of whether to render a route immediately and whether to change to a different route with optionally the original route parameters themselves. Most authentication is global, for UX if nothing else, and it's much easier to break apart a route than it is to resolve a route. This also makes things like sign-in pages for restricted routes much easier to do.
    • If your architecture is properly componentized, you shouldn't need to propagate the parameters very far to begin with. If it's not, it's at least forcing you to be more explicit what state you need, which makes your components easier to test. This is enforcing an opinion, something Mithril usually avoids, but it's just pushing you to follow best practices most JS front-end frameworks would want you to do anyways, just to a deeper level.
    • By making m.route.params() a local, you're saving a few characters doing it in the two places you're likely using it the most: the global page layout and the top-level route-specific layouts. You could even destructure them if you only need a few of them.
    • One could implement a component-specific onmatch by just calling the route with a magic non-string parameter value (like render(null, {...currentParams, checkAuth: true})) and checking for it there (like params.checkAuth === true). That allows easy checking without even having to explicitly code support for it.

Of course, there are cons:

  • This isn't exactly codemoddable. One could create a temporary shim for the old API so people can codemod to the shim while they work on removing it, and the shim is not as hard as it'd seem at first glance, but the shim is still necessary for migration.
  • This is a substantial departure from what we currently have, so people will have to relearn the routing system. Unlike the component system, this didn't substantially change from v0.2.
  • Creating links for more complicated cases (like routable <div>s) is now a little more boilerplatey to set up, but I doubt that was very common. (It does simplify the common case, however.)
  • Router initialization is slightly more syntax-heavy than it once was.
  • This increases the router size slightly - about 200 bytes gzipped, compared to about 1.1K gained by removing the router altogether. (To be fair, it also adds a few features that didn't exist previously.)

And finally, of course, this could all be a terrible idea, so I'm not too attached to it.

@barneycarroll
Copy link
Member

barneycarroll commented Jul 9, 2018

@isiahmeadows three really neat things about your proposal:

  1. The router function produces a component, de-duplicating the (dom, content) signature, making the 'Layout' pattern obsolete
  2. The central resolve function allows global routing concerns to be addressed DRYly, makes route resolvers obsolete
  3. The mapping of route: view instead of route: component | resolver eliminates boilerplate, increases flexibility

You've clearly thought about separation of concerns a fair bit, but despite the optimisations above and beyond pluggable back end, I still think your m.route.init is doing too many things - as are the other proposals. Significantly, I think the global initialisation configuration should be separate from the entity that consumes route maps and produces virtual DOM. The former needs to happen, once, before the latter can take effect (any number of times). How about breaking it up as follows:

const { Route, Link } = m.router({
  prefix,
  onhashchange,
  initial,
  resolve,
  // ^ all optional
})

m.mount(document.body, {
  view: () => [
    m(Nav),

    m(Link, {href: '/'}, 'Home'),

    m(Route, {
      '/': (...data) =>
        m('h1', 'Hi!'),

      ...etc,
    }),
})

The Route component seems odd at first coming from Mithril or whatever, but in React world it's completely intuitive. When I started playing with applying this concept to Mithril I discovered that some things weren't as powerful as I'd imagined (multiple route maps in different places), but other things (the 'compound component' / 'provider' system favoured by React training) weren't that sensible either: you're dependent on a central controller in order to produce route: view hash components and links, but having that controller as a component too doesn't make much sense outside of an 'idiomatic context API' culture - the configuration and back end is essentially orthogonal to the virtual DOM concerns.

I might try and bake these proposals above into Moria 2 so we can test drive the proof of concept. https://github.com/barneycarroll/moria

@dead-claudia
Copy link
Member

@barneycarroll BTW, that dynamic routing idea is basically what React Router does. I see how it's useful, and I do like the abstraction. React Router also explains at length in that link why that's useful and in some cases necessary, but the implementation is deceptively harder to optimize, which is why I was hesitant to suggest it.

If I were to go down that route (no pun intended), I think the API might be better as this:

  • m.route.get(vnode) -> path - The current route.

  • m.route.set(vnode, path, options = {}) -> params - As I previously proposed m.route.set.

  • m.route.params(vnode) -> params - The raw query parameters. It doesn't include route-specific parameters, however - those are only available in the routes' view methods' arguments themselves.

  • m(m.route.link, {to, tag = "a", attrs?, ...options}, ...children) - As I previously proposed m.route.link, just bound to this router.

  • m(m.route, {default: "/", routes, fallback?, current?, prefix?, redraw?, register?}) - Define the root route.

    • default is the default route.
    • routes are the routes you wish to render.
    • prefix is the prefix, replacing m.route.prefix.
    • current, if present, is the full path to render, without the prefix and independent of the global path itself. The default is inferred from register + the prefix, indirectly from window.location if the default register is used. This is for three things:
      1. It replaces m.route.set redirects and the return value of onmatch - you can just use this instead.
      2. It's easier to test - you don't need to imperatively do anything to set it.
      3. It works with server-side rendering - you don't need to bring a fully-fledged DOM just to render it.
    • Future: redraw and register are as I proposed above.
    • Future: this may be nested within another m(m.route, {...}), in which it acts as a child router.

And of course, there are a couple non-routing things we would need to add to support the above:

  • m.context.set(key, value, tree) - Set a hidden context key/value pair separate from the tree. This is made available and normalized through the vnode tree during rendering via vnode.context as vnode.context[key] = value, but is also retrievable via m.context.get(key, callback). These are propagated via frozen prototype clones, with the root instance is a frozen Object.create(null), so you don't have prototype interference.

    • This value is not persisted. It's just used for that render.
    • When building the context, observably identical values are not added, but only delegated.
    • This is required for the router, but others might find it useful for things like server-side rendering, too, where one might want to differentiate between a server-rendered template and a client-rendered one.
    • Note: it's advised to document your context key(s) (exact or pattern) as an API note to avoid clashing, if it's not a local symbol.
    • Note: it is advised not to expose your context keys' value as part of your public API. Instead, you should expose wrapper functions accepting vnodes, so it's less verbose for your users. It also helps cut down on how much context you need to use - you might be able to inexpensively compute some of what you need to expose, meaning you can keep that out of the context.
    • Trust me: this is way simpler than the React equivalent - it's more dynamic and fluid and it doesn't require you to generate an entire component for a simple vnode-like construct. It works even without components by design. It's also simpler to implement - it's just finding all the callees to add the relevant arguments, beyond the explanation above.
  • m(m.async, {redraw?, expire?, resolve?: (onExpire) => ..., loading?: (resolved) => ..., view: (result) => ...}) - Define an async component.

    • resolve is called on first load within oninit, and can return either the immediate result to call view with or a promise to it. If it returns a promise, the promise is awaited and that result is used on the next call to view on the next redraw, which is scheduled immediately.
    • view receives the direct result of resolve, resolved and unwrapped, and returns an optional promise to a vnode tree. If it returns a promise, it is awaited and that resolved tree is rendered on the next redraw, which is scheduled immediately.
    • loading is called with resolved if the resolve promise has already resolved, and the return value is used as the interim vnode tree while you wait on either resolve, the view, or both.
    • Future: Pass expire: true to expire any existing resolution and force it to reinitialize. In addition, result is set to undefined so the view loses access, and the last registered onExpire(() => ...) callback is called so the previous resolve call can clean up if it needs to.
    • Future: Pass redraw to do a custom redraw. This may seem redundant on its face, but it's useful in tandem with my mithril-helpers/self-sufficient component, where you could pass redraw: () => state.redraw().
    • This component exists mainly to supplant the onmatch part of route resolvers, but also to enable a few other potential things like lazily loaded routes (!).
    • This works very similarly to this React component, but it's much more focused and broadly useful.

Now here's some of the things this permits beyond the obvious (@barneycarroll you probably already know some of these):

  • You get easy, trivial lazy route loading. You simply wrap the router in a m(m.async, {...}). If you want to prioritize data, you can do that by just requesting those before anything else. And yes, if the child router concept is included, you can even lazily load child routes entirely this way.
  • As explained by the React Router documentation, you can create device-specific routing, given the appropriate components.
  • If child routes are included, third-party components can now create their own routing systems without interfering with the parent routing. Yes, routes are composable this way.
  • You could create support for theming without explicit support within your app. The only difference is that you either expose components with the elements you need (best), or you expose functions to get or set that data. (You could do similar with languages, too.)
  • You can branch based on server-side or client-side rendering without having to check a global variable or pass an attribute around. Instead, you could use a context variable to just know "hey, this is server-side" or "hey, this is client-side", and use that to determine what data you present.

And yes, I think React Router has probably pretty close to the ideal API for routing. They at least hit the part of dynamic routing, but their over-reliance on JSX for defining routes makes me think ColdFusion and XSLT, not JS. (And although it does permit easy conditional route definitions, it's pretty useless to go that low-level, especially for something static enough.)


I have a feeling this is probably still hanging on to a little too much bloat, so if anything here could be safely stripped without eliminating existing use cases, I'm good with it.

@kczx3
Copy link
Contributor

kczx3 commented Feb 15, 2019

Would love to see routing turn into something more like what @barneycarroll describes. The routing in Mithril is one of the things that keeps me from jumping in head first. React Router really does seem so much more intuitive and the routes can be spread all across the app instead of all centrally located.

@dead-claudia
Copy link
Member

BTW, regarding the original bug, it appears their behavior is broken only sometimes. It also appears to have existed at one point in Edge.

@dead-claudia
Copy link
Member

Closing due to age.

@dead-claudia dead-claudia closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2024
@dead-claudia dead-claudia moved this to Completed/Declined in Feature requests/Suggestions Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed/Declined
Development

No branches or pull requests

5 participants