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

Partially recast the router API to be a lot more intuitive. #2469

Merged
merged 5 commits into from Jul 12, 2019

Conversation

@isiahmeadows
Copy link
Member

@isiahmeadows isiahmeadows commented Jul 12, 2019

Description

  • m.route.prefix(...)m.route.prefix = ...
  • oncreate: m.route.linkm(m.route.Link, ...)
  • Add m.route.SKIP

Motivation and Context

Fixes #2387
Fixes #2072
Fixes #1792
Fixes #2354 (not in the way they wanted, but it at leasts unblocks them)
Fixes quite a few issues reported on Gitter.

For m.route.Link:

  • More intuitive
  • More accessible
  • More ergonomic
  • It can be disabled
  • It can be cancelled
  • It can be changed
  • Oh, and you can use it isomorphically.

For m.route.prefix

  • You can read it.
  • You can write to it, of course.
  • It's literally just setting a property.

For m.route.SKIP

  • You can type-check routes, now.
  • You can hide routes and pretend they don't exist.
  • You can check existence of something and fall back later.

For the router itself (and the rest of Mithril):

  • You can now require("mithril") and all its submodules without a DOM at all. There is a catch: you can't instantiate any routes, you can't mount anything, and you can't invoke m.render in any capacity. You can only use m.route.Link, m.route.prefix, hyperscript stuff, and mithril/stream, and you can use m.request with background: true if you use a global XHR polyfill. (You can't use m.request without background: true except with a DOM to redraw with.) The goal here is to try to get out of the way for simple testing and to defer the inevitable TypeErrors for the relevant DOM methods to runtime.

    The factory requires no arguments, and in terms of globals, you can just figure out based on what errors are thrown what globals to define. Their values don't matter - they just need to be set to something, even if it's just null or undefined, before Mithril executes.

Had to make quite a few other changes throughout the docs and tests to
update them accordingly. Oh, and that massive router overhaul enabled me
to do all this.

Also, slip in a few drive-by fixes to the mocks so they're a little
easier to work with and can accept more URLs. This was required for a
few of the tests.

How Has This Been Tested?

Added and updated a lot of tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md
@project-bot project-bot bot added this to Needs triage in Triage/bugs Jul 12, 2019
Fixes MithrilJS#2387
Fixes MithrilJS#2072
Fixes quite a few issues reported on Gitter.

For `m.route.Link`:

- More intuitive
- More accessible
- More ergonomic
- It can be disabled
- It can be cancelled
- It can be changed
- Oh, and you can use it isomorphically.

For `m.route.prefix`

- You can *read* it.
- You can write to it, of course.
- It's literally just setting a property.

For the router itself (and the rest of Mithril):

- You can now `require("mithril")` and all its submodules without a DOM
  at all. There is a catch: you can't instantiate any routes, you can't
  mount anything, and you can't invoke `m.render` in any capacity. You
  can only use `m.route.Link`, `m.route.prefix`, hyperscript stuff, and
  `mithril/stream`, and you can use `m.request` with `background: true`
  if you use a global XHR polyfill. (You can't use `m.request` without
  `background: true` except with a DOM to redraw with.) The goal here is
  to try to get out of the way for simple testing and to defer the
  inevitable `TypeError`s for the relevant DOM methods to runtime.

  The factory requires no arguments, and in terms of globals, you can
  just figure out based on what errors are thrown what globals to
  define. Their values don't matter - they just need to be set to
  *something*, even if it's just `null` or `undefined`, before Mithril
  executes.

Had to make quite a few other changes throughout the docs and tests to
update them accordingly. Oh, and that massive router overhaul enabled me
to do all this.

Also, slip in a few drive-by fixes to the mocks so they're a little
easier to work with and can accept more URLs. This was required for a
few of the tests.
@isiahmeadows
Copy link
Member Author

@isiahmeadows isiahmeadows commented Jul 12, 2019

Apologies for the very large diff on api/tests/test-router.js - it's all intended to actually test for how ticks are fired. I was previously running into weird sequencing issues and having to add and subtract a lot of callAsyncs with varying levels of success (mostly fleeting at best), so I just switched to what m.route was using internally and relying directly on the number of ticks needed. They're easily adjusted, and I left a comment so if anyone needs to adjust them, they have an idea of what to do. (The ticks are intended to be lower precedence than promise microtasks, and the tests may break on IE. I also need to test them with the polyfill and with the hashchange fallback at some point.)

@isiahmeadows isiahmeadows merged commit 582bda5 into MithrilJS:next Jul 12, 2019
1 check passed
Triage/bugs automation moved this from Needs triage to Closed Jul 12, 2019
@isiahmeadows isiahmeadows deleted the update-router branch Jul 12, 2019
@spacejack
Copy link
Contributor

@spacejack spacejack commented Jul 14, 2019

Sorry to point this out after the fact but was pondering the component attr name. In the hyperscript docs this parameter is referred to as the selector. I wonder if selector would be a better name for the attr?

I was also wondering if tag would be a better name. But we can't use that (I don't think?) because it would be interpreted as a vnode (child) rather than an attrs object. Which makes me wonder if it would be useful re-naming the vnode.tag property to something like vnode.__TAG to free up the name tag for application use in attrs.

@isiahmeadows
Copy link
Member Author

@isiahmeadows isiahmeadows commented Jul 16, 2019

@spacejack I wouldn't be against it in the future, but I'd like to flesh out changes to the vnode structure in a separate bug. Short term, I'm correcting the design bug (componentselector) in a followup PR. Edit: #2475

@isiahmeadows isiahmeadows mentioned this pull request Jul 16, 2019
11 tasks
@soaxelbrooke
Copy link

@soaxelbrooke soaxelbrooke commented Jul 24, 2019

Sorry if this isn't the right place to ask, but I couldn't find an answer in the docs. I've got a lot of m('a.someclass', { href, oncreate: m.route.link })s sitting around my codebase, and this is the only breaking change I need to accommodate. Should those be replaced with m(m.route.Link, m("a.someclass", { href }))?

@osban
Copy link
Collaborator

@osban osban commented Jul 24, 2019

@soaxelbrooke I think it should be m(m.route.Link, {href, class: 'someclass'})
Oh, you could also ask it in the Mithril Gitter chat.

@isiahmeadows
Copy link
Member Author

@isiahmeadows isiahmeadows commented Jul 26, 2019

@soaxelbrooke Either use class: "someclass" or selector: "a.someclass", whichever you prefer.

If anyone wants to update the docs to clarify this, I'd be more than willing to accept a pull request.

@ancms2600
Copy link

@ancms2600 ancms2600 commented Sep 1, 2019

Minor suggestion: add a bypass:boolean attribute to the m.route.Link component.
moved to: #2525

@isiahmeadows
Copy link
Member Author

@isiahmeadows isiahmeadows commented Sep 4, 2019

@ancmikesmullin For disabling internal links, consider using disabled: true on m.route.Link - that lets you specifically disable a link, and it does so accessibly. If you really intend to disable routing while still reacting to left clicks, you'll need to manually add a click listener yourself in oncreate and/or onupdate.

For the case of external links, please file a new issue for that. I'd definitely be willing to investigate a solution to that, but only separate from this. Do note that nobody else does that, so we'd be among the few that would. (I wouldn't be against it, but I do want to make sure it's reasonably intuitive and that it's done well the first time.)

@Niklas81
Copy link

@Niklas81 Niklas81 commented Sep 5, 2019

Since Mithril started using m.redraw defined as m.redraw = mountRedraw.redraw, it crashed Node when running Mithril server-side.

I'm using Mithril isomorphically, and these changes, specifically the usage of requestAnimationFrame in mount-redraw.js, created an interesting exercise in hacking my mithril-wrapper. Perhaps there is a point in making this more Node-friendly by default?

For example, we could have this in mount-redraw.js (or some better solution perhaps?):


var render = require("./render")

module.exports = require("./api/mount-redraw")(render, process.browser ? requestAnimationFrame : () => {}, console)

Just to give an idea of what I had to tweek in order to make version 2.0.4 work after the upgrade:

var m = process.browser ? require('mithril') : null
var parseQueryString = !process.browser ? require('mithril/querystring/parse') : null
var buildQueryString = !process.browser ? require('mithril/querystring/build') : null

if (process.browser) {
  module.exports = m
}
else {
  const noop = () => {} // function that returns an empty function
  module.exports = nodeRequire('mithril/hyperscript')
  var render = require('mithril/render')
  var mountRedraw = require('mithril/api/mount-redraw')(render, null, console)
  var mRoute = require('mithril/api/router')(window, mountRedraw) // m.route
  mRoute.prefix = ''
  module.exports.route = {
    get: function(){ return global.route.get },
    param: function(key){ return !key ? global.route.param : global.route.param[key] },
    Link: mRoute.Link,
    lock: {status: noop}
  }
  module.exports.redraw = noop
  module.exports.parseQueryString = parseQueryString
  module.exports.buildQueryString = buildQueryString
}

@isiahmeadows
Copy link
Member Author

@isiahmeadows isiahmeadows commented Sep 6, 2019

@Niklas81 You just need to define a few globals to anything, even if undefined, to remove the inevitable ReferenceErrors, and then things should just work as long as you aren't actually rendering anything. Ideally, I'd like to move to a pure non-global implementation using document = root.ownerDocument + window = document.defaultView, but that's a bit more involved of a change.

@isiahmeadows
Copy link
Member Author

@isiahmeadows isiahmeadows commented Sep 6, 2019

But be sure to define those globals before loading Mithril. That's the key.

@Niklas81
Copy link

@Niklas81 Niklas81 commented Sep 7, 2019

Thanks, to keep thinks even simpler and located in the same place in my codebase, you made me realize I can simply replace this:

var render = require('mithril/render')
var mountRedraw = require('mithril/api/mount-redraw')(render, null, console)
var mRoute = require('mithril/api/router')(window, mountRedraw) // m.route

with this:

var mRoute = require('mithril/api/router')() // m.route

@joshuajameshunt
Copy link

@joshuajameshunt joshuajameshunt commented Sep 22, 2019

I motion against built-in components like m.route.Link as a rule regardless of how "ergonomic" they are. oncreate:m.route.link or an onclick: function is just as easy and keeps the framework cleaner.

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