MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
13.98k stars 925 forks source link

[suggestion] Legacy router mode #2179

Closed pygy closed 6 days ago

pygy commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

pygy commented 6 years ago

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

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

barneycarroll commented 6 years ago

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

orbitbot commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

... 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 commented 6 years ago

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

pygy commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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.
    },
}))

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

Of course, there are cons:

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

barneycarroll commented 6 years ago

@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 commented 6 years ago

@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:

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


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

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 commented 5 years ago

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 commented 5 years ago

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 commented 6 days ago

Closing due to age.