v3 proposal #2982
v3 proposal #2982
Conversation
Perf is somewhat improved in spots, but generally the same otherwise.
Cuts only about 2% of the bundle, but removes a pretty bad pain point in the code. Plus, inserting arbitrary HTML outside a container is usually a recipe for disaster in terms of styling.
It's simpler that way and I don't have to condition it.
It's redundant and has been for ages. A v3 increment gives me the ability to minimize surprise.
…t from the router
People can just use equality and regexps as needed.
Also, rename it `m` to make dev stack traces a little more readable, and remove a useless layer of indirection.
This makes that class fully monomorphic and also a bit safer.
That's not been there for years
The long comment at the top of it explains the motivation.
It's now been made redundant with `m.tracked`.
`finally` is apparently really slow. This improved benchmarks by about 3-4x, bringing them back to roughly what they were when I first cut the branch.
This matches HTML more closely, and makes for a much better experience with custom elements.
It's more predictable that way, and I expect this to break approximately nobody. (If anything, it's likely to *unbreak* some users, as they may have been assuming this to have been the case from the beginning.)
Some point along the way, this got erroneously changed and saved.
It's going to unnecessarily complicate the release process, especially now that artifacts aren't saved to the repo anymore.
dead-claudia
added
Type: Breaking Change
|
First off, thanks @dead-claudia for this massive push. I will try to add comprehensive review hopefully this week. |
|
I'm missing a trick on how routing with params should work now? In the past I had m.route.prefix = '?'
m.route(rootElement, '/', {
'/': { view: () => m(RootView, { context }) },
'/clients': { view: () => m(ClientIndexView, { context }) },
'/clients/:clientId/connect': { view: () => m(ClientConnectView, { context }) },
})Which worked to both generate and consume routes with params. Now I've got function App(_attrs, _old, {route}) {
switch (route.path) {
case "/": return m(RootView, { context })
case "/clients": return m(ClientIndexView, { context })
case "/clients/:clientId/connect": return m(ClientConnectView, { context })
default: route.set("/")
}
}
m.mount(rootElement, () => m(m.WithRouter, { prefix: '?' }, m(App)))But clearly that case statement won't work because Is there a new way to specify the |
I used function App(_attrs, _old, {route}) {
let match
if (route.path === "/") {
return m(RootView, {context})
}
if (route.path === "/clients") {
return m(ClientIndexView, {context})
}
if (match = /\/clients\/([^/]++)\/connect/.exec(route.path)) {
return m(ClientConnectView, {context, clientId: decodeURIComponent(match[1])})
}
route.set("/")
}If you wanted to completely abstract it away so you could still have if (match = /\/clients\/([^/]+)\/connect/.exec(route.path)) {
route.params.set("clientId", decodeURIComponent(match[1]))
return m(ClientConnectView, {context})
}Be mindful that I'm thinking of adding something like |
|
I was worried ES2018 might break Pale Moon, but just tested the counter example that seems to work. 👍 |
|
I've been waiting for this proposal @dead-claudia. Been some years you've been hacking on it, cool to see it reach this level. Can't wait to get some time and dig into it. |
|
Great job. I will touch it later. |
|
First, thanks again @dead-claudia for the work you put into that PR. I really like the way you're heading with the move away from magic-props to special vnodes. This will greatly improve composability on the lifecycle front. I still don't fully understand, how some of the new special vnodes work, esp. What I'm skeptical about
Another thing that I'm worried about is the amount of breaking changes and the backwards compatibility. As you might have noticed the mithril community is not in a great shape. Most of the main contributors (community and code) have moved away to other projects and a lot of the remaining ones have build massive apps that rely on a stable API, which is a big strength of mithril and allows for such big projects. So maybe it would be a good idea to keep some of the old ergonomics and deprecate them for at least one major version, so people have time to switch to the newer way of doing stuff. And maybe it turns out that it does not live up to it's expectations and all people like the old API more. I would at least keep So hope this helps. |
|
One thing I forgot to ask: What's the SSR story of v3? I would really like to discontinue mithril-node-render and see it integrated into the main framework. And the question the comes next: How is the support for hydration of SSR-Apps? |
I want your feedback. Please use it, abuse it, and tell me what issues you find. Bugs, ergonomic issues, weird slowdowns, I want to know it all. Feel free to drop a comment here on whatever's on your mind, so I can easily track it.
Also, @MithrilJS/collaborators please don't merge this yet. I want feedback first. I also need to create a docs PR before this can get merged.
Description
This is my v3 proposal. It's pretty detailed and represents a comprehensive overhaul of the API. It synthesizes much of my API research. And yes, all tests pass. 🙂
Preserved the commit history if you're curious how I got here. (Reviewers: you can ignore that. Focus on the file diff - you'll save yourself a lot of time.)
If you want to play around, you can find artifacts here: https://github.com/dead-claudia/mithril.js/releases/tag/v3.0.0-alpha.0
This is not published to npm, so you'll have to use a GitHub tag/commit link. If you just want to mess around in a code playground, here's a link for you.
Quick line-by-line summary
If you're short on time, here's a comprehensive list, grouped by type.Highlights:
Additions:
m.layout((dom) => ...), scheduled to be invoked after renderm.remove((dom) => ...), scheduled to be invoked after the render in which it's removed fromm.retain(), retains the current vnode at that given positionm.set(contextKeys, ...children), sets context keys you can get via the third parameter in componentsm.use([...deps], ...children), works similar to React'suseEffectm.init(async (signal) => ...), callback return works just like event listenerstracked = m.tracked()to manage delayed removalm.throttler()m.debouncer()m.withProgress(stream, onProgress), replaces oldm.request'sonprogressChanges:
key:attribute ->m.key(key, ...)m.buildPathname(template, query)->m.p(template, query)m.buildQueryString(query)->m.q(query)falseto prevent redraws.removeOnThrow: falseis passed tom.render.removeOnThrow: falseis passed tom.render.m.route(...)->m(m.WithRouter, {prefix}, ...children)m.mount(...)now returns aredrawfunction that works likem.redraw, but only for that one root.redrawis also set in the context via theredrawkey.m.render(...)now accepts an options object for its third parameter.redrawsets the redraw function (and is made available in the context).(attrs, old, context) => vnodefunctions or an(attrs, null, context) => ...function returning thatRemovals:
mithril/stream- the main library no longer needs the DOM to loadm.trust- useinnerHTMLinsteadm.censorm.request- usefetch(m.p(template, query))and (if necessary)m.withProgress(...)insteadm.vnode(...)- usem.normalizeif it's truly necessarym.mount(root, null)- usem.render(root, null)insteadm.redraw()-> eitherredrawfunction returned fromm.mountor theredrawcontext keyoninitoncreate/onupdate- usem.layout(...)insteadonremove- usem.remove(...)insteadonbeforeupdate- usem.retain()insteadonbeforeremove- usem.tracked()utility to manage parents insteadm.parsePathnamem.parseQueryStringev.redraw = false- return/resolve withfalseor reject/throw from event handlersMiscellaneous:
mithril/streamto require a lot less allocation - I don't have precise numbers, but it should both be faster and require less memory.Benchmarks
Details about my setup and related raw data follow the table.
simpleTreenestedTreemutateStylesPropertiesTreerepeatedTreeshuffledKeyedTreesimpleTreenestedTreemutateStylesPropertiesTreerepeatedTreeshuffledKeyedTreesimpleTreenestedTreemutateStylesPropertiesTreerepeatedTreeshuffledKeyedTreesimpleTreesimpleTreeBrowser version:
This was run in a private window, with only the following two extensions:
Raw output for benchmarks in this PR, ported to v2.2.8:
Raw output for benchmarks in this PR, using the current:
Motivation and Context
This resolves a number of ergonomic issues around the API, crossing out many long-standing feature requests and eliminating a number of gotchas.
Related issues
- Fixes https://github.com//issues/1937 by enabling it to be done in userland, mostly via `m.render(elem, vnode, {removeOnThrow: true})` - Resolves https://github.com//issues/2310 by dropping `m.trust` - Resolves https://github.com//issues/2505 by not resolving routes - Resolves https://github.com//issues/2531 by not resolving routes - Fixes https://github.com//issues/2555 - Resolves https://github.com//issues/2592 by dropping `onbeforeremove` - Fixes https://github.com//issues/2621 by making each mount independent of other mount points - Fixes https://github.com//issues/2645 - Fixes https://github.com//issues/2778 - Fixes https://github.com//issues/2794 by dropping internal `ospec` - Fixes https://github.com//issues/2799 - Resolves https://github.com//issues/2802 by dropping `m.request`Related discussions
- Resolves https://github.com//discussions/2754 - Resolves https://github.com//discussions/2775 by not resolving routes - Implements https://github.com//discussions/2912 - Implements https://github.com//discussions/2915 by throwing - Resolves https://github.com//discussions/2916 by dropping `m.request` - Implements https://github.com//discussions/2917 with some minor changes - Implements https://github.com//discussions/2918 - Implements https://github.com//discussions/2919 - Implements https://github.com//discussions/2920 - Resolves https://github.com//discussions/2922 by dropping `m.request` - Resolves https://github.com//discussions/2924 by dropping `m.request` - Implements https://github.com//discussions/2925 - Resolves https://github.com//discussions/2926 by dropping `m.request` - Resolves https://github.com//discussions/2929 by not doing it (doesn't fit with the model) - Resolves https://github.com//discussions/2931 by not resolving routes - Resolves https://github.com//discussions/2934 by dropping `m.request` - Resolves https://github.com//discussions/2935 by not resolving routes - Partially implements https://github.com//discussions/2936 - Partially implements https://github.com//discussions/2937, intentionally skips rest - Resolves https://github.com//discussions/2941 by not resolving routes - Implements https://github.com//discussions/2942 by offering a configurable `route` context key - Implements https://github.com//discussions/2943 - Implements https://github.com//discussions/2945 - Resolves https://github.com//discussions/2946 by making the router a component instead (thus making it implicit) - Implements https://github.com//discussions/2948 by throwing - Resolves https://github.com//discussions/2950 by dropping `m.request`Things this does *not* resolve
- https://github.com//issues/2256 (This needs some further digging to lock down precisely what needs done) - https://github.com//issues/2315 - https://github.com//issues/2359 - https://github.com//issues/2612 - https://github.com//issues/2623 - https://github.com//issues/2643 - https://github.com//issues/2809 - https://github.com//issues/2886New API
This is all in the style of https://mithril.js.org/api.html.
m(selector, attrs, children)
m.mount(element, component)
context.redraw()
m(m.WithRouter, {prefix}, ...children)
route.set(path)
route.current
m(m.Link, ...)
m.p(template, params)
m.q(object)
How Has This Been Tested?
I've written a bunch of new tests, and every current test (to the extent I've kept them) pass. At the time of writing, there's 8929 tests.
Types of changes
Checklist