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
Making Mithril modular #665
Conversation
+1 for this. |
👍 from me too. Apart from anything else, this makes the source far easier to reason about. |
Relative sizes:
I used the commit immediately prior to this PR and I used I agree with @barneycarroll. Reasoning goes up by a lot more than 10%. However, it's worth noting that this PR adds some non-trivial heft to Mithril. I don't understand what Webpack is doing under the covers, but is there a way to produce a "cheap" build that resembles the current format? |
|
||
var to = Object(target); | ||
for (var i = 1; i < arguments.length; i++) { | ||
var nextSource = arguments[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only really need a simple mixin function, not the whole polyfill here. This whole module could simply be the following.
'use strict';
module.exports = function (target) {
for (var i = 1; i < arguments.length; i++) {
var obj = arguments[i];
for (var prop in obj) {
if (Object.prototype.hasOwnProperty.call(obj, prop)) {
target[prop] = obj[prop];
}
}
}
return target;
};
This should also noticeably cut down on the boilerplate.
Also, this is only used in the main script, so it could just as easily be made local to the script.
@venning Could you test these "src/index.js" minified? (these substitute lines 1-9) // This is the original - it's already tested above
var assign = require('./helpers/Object.assign');
var m = require('./modules/DOM');
assign(m,
require('./modules/router'),
require('./modules/utils'),
require('./modules/http'),
require('./modules/mock')
); // 1. just DOM
var m = require('./modules/DOM'); // 2. with routing, mock
var assign = require('./helpers/Object.assign');
var m = require('./modules/DOM');
assign(m,
require('./modules/router'),
require('./modules/mock')
); // 3. no HTTP/async utils
var assign = require('./helpers/Object.assign');
var m = require('./modules/DOM');
assign(m,
require('./modules/router'),
require('./modules/utils'),
require('./modules/mock')
); // 4. no router, but with async utils
var assign = require('./helpers/Object.assign');
var m = require('./modules/DOM');
assign(m,
require('./modules/utils'),
require('./modules/http'),
require('./modules/mock')
); |
var noop = function(){}; | ||
|
||
function propify(promise, initialValue) { | ||
var prop = m.prop(initialValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you might want to add this to the top: var m = require('../modules/utils')
. Otherwise, it's broken unless m
is globally installed.
Also, much of the modularization work includes some extra, and in a couple cases, unnecessary, boilerplate as well. A couple examples of this:
module.exports = function (target) {
for (var i = 1; i < arguments.length; i++) {
var object = arguments[i];
for (var prop in object) {
if (Object.prototype.hasOwnProperty.call(object, prop)) {
target[prop] = object[prop];
}
}
}
return target;
}; Another is in "src/modules/mock.js", where it could just as easily be rewritten as this: var $ = require('../core/init');
exports.deps = function(mock) {
$.initialize(window = mock || window);
return window;
}; And to be honest, this is starting to feel a little heavier, after the almost Promises/A+-conforming deferred implementation (that I feel could be done without, given incoming ES2015 promises), and the request API in which a wrapper is trivial to create. I don't see the need for those two in Mithril's primary exports, although an npm module + browser bundle would be completely appropriate. // Theoretical API
var m = require('mithril');
require('mithril-http')(m);
// You now have those APIs on `m` var m = require('mithril');
var h = require('mithril-http')({});
// You have those APIs on `h`, separate from Mithril's exports As for how |
@IMPinball I agree, the original PR was submitted prior to implementing several possible optimizations/changes, as I felt the additions made are reasonable at least for a PoC. I'll incorporate the suggested changes (or a PR if you open one) this weekend. @venning I chose webpack as it has the lightest boilerplate that lends well to minification. I don't think I've seen a way to reduce the boilerplate code size further. All it does is wrap each module (file) in a function that gets called when you require that module. |
@mosho1, how are you building this? I haven't looked at it too closely, but it looks like the |
@venning Caused by using a later version of grunt-webpack than what was used in the initial commit. I've updated the gruntfile to reflect the change. Relevant issue: webpack-contrib/grunt-webpack#45 |
Thanks, @mosho1, I got it to build with that update, which also modified the outputted @IMPinball, here are the results of your experiments:
I did not attempt any of the other tweaks, such as replacing the |
I'm looking at the fact removing the HTTP utils is really nice for people who want to use something else for Ajax, and the DOM-only version gives the speed benefits to those who are using other frameworks, while letting them stick with the opinions of the other framework. Some people use it in a similar sense to React, as just the V in MV*. (I saw someone use Mithril as a view template for an Angular app, for example.) |
@IMPinball I wonder if it also going to be easier to incrementally migrate an Angular app to Mithril app. |
I could make an argument for even more breakdown within Also, it looks like some of |
Good point. I'm tempted to provide my own PR that fixes a lot of these practical minification problems (dead code, aliasing module.exports, etc.) |
Although another thing I would do is retrofit the tests to be runnable in Node. That is equally important IMO, and shouldn't be that hard to do. |
Thanks for the input guys. I've incorporated several of the optimizations mentioned and the fix @IMPinball mentioned to fns.js The stats now are:
DOM only is 12,927 minified and 5,093 gzipped. |
This is really exciting. Is there a way to factor out Mithril deferred so it can be used sans |
For the record, I'm a bit against this as it stands. I have a few concerns, listed below. For those of us using the full Mithril package, this only adds bloat and an (admittedly slight) initilization slowdown. Perhaps I am misreading Mr Horie's intentions, but this seems to be the opposite of the Mithril mentality of light, fast, and concise. If the concern is how readable If the concern is selectively removing unused functionality, then there are a couple things worth noting about that: that's a common desire for probably every framework and not a well-solved one; and Mithril does not demand usage of its "extraneous" parts. There shouldn't be any impact to developers of having unused functionality as a part of Mithril. Happens with every library. Even in the best case, this feels a bit like taking one step back before two steps forward. I'd prefer to avoid the regression. If you'd truly like a cut-down Mithril for the sake of size, packaging it up like this isn't helping since you still wind up with more boilerplate than strictly necessary. I guess this is my point: Splitting the router out of Angular 1.x made sense as it was already a large library to begin with and removing oft-unused functionality helped Angular with addressing one of its problems: size. Mithril doesn't have size as a problem. Mithril has size as a virtue. As such, I don't like damaging one of Mithril's virtues for the sake of catering to specific use cases. Okay, I don't want to leave this rant without suggesting a potential solution. I would be in favor of leaving Thoughts? |
@pelonpelon That should be quite possible. Splitting things into modules is just a matter of refactoring, if at all. In this case extracting @venning I can see what you mean. An alternative I could offer (basically what you suggested) is a build with just concatenation. Then we could have a |
And to be perfectly honest, although I like the idea of this getting divided into modules, I'm not sold on this particular implementation, anyways. The layout isn't really that great, the format of each module isn't really standard, and there are a few other things that would need fixed to make the module separation cleaner and better. There are a few misplaced functions as well, and higher coupling tends to increase build size, especially in this type of scenario. @mosho1 Pure concatenation would still have to prioritize the DOM implementation first, so the programmer would have to do some of the linking work otherwise done by the compiler. And as for Webpack, the size difference IMHO was cooked up a little more than it should have been, but this particular implementation didn't really help that. |
@lhorie we missed you :) this is great news — sounds awesome!
I think this is a crucial aspect of this PR. I'm in no way suggesting Mithril's "everything you need to build an SPA" approach is a cardinal sin, but, for example, I think it's a bit of a pyrrhic task trying to improve Mithril's XHR when I could just drop that module from my build and use a |
@lhorie : "Granular redraws" - Will it allow efficient modals, e.g. rendering a "Login Modal" over an expensive-to-render page? I think it would be easily possible to just stack the var superRoots = [];
function pushModal(root, component) {
superRoots.push(roots);
roots = []; // this is the roots variable from mithril
m.mount(root, component);
}
function popModal() {
// unmount?
roots = superRoots.pop();
} btw, as you mentioned cito.js, I just switched back from cito.js to mithril :). Cito is great, but I found myself reinventing the wheel all the time. |
@IMPinball : I also like the minimalist approach of mithril and it's great documentation. I wonder if the performance comparison is using something like the Mithril sweet.js macros. Because in cito.js you usually use plain Javascript object to define your tags, not calling any methods. |
It seems that rollup.js should be used for modularization instead of Webpack. It generates almost no boilerplate and allows access Mithril parts for powerusers while maintaining single core. |
I already like that. It does full ES6 module resolution. How stable is it? On Thu, Oct 29, 2015, 14:03 Kolya Ay notifications@github.com wrote:
|
I don't think that rollup has already reached "enterprise quality" in the sense of essential plugins and third party tools integration. However it should be quite decent for such selfcontained no deps project as Mithril. The most important thing is that It does job right |
very good. |
FWIW, rollup is used for building Google's incremental-dom. |
@pygy Seems like a one small step to enterprise admission;) |
If anybody is interested, JSPM contains a ES6 module loader. |
@veggiemonk all my Mithril component files start with import m from 'mithril' There is currently no native implementation for ES6 modules. The community standard tool for transpiling ES6 to ES5 is Babel, which translates |
exactly as @barneycarroll is saying... https://github.com/geut/mithril-transition/tree/master/example |
Given incremental-dom is using it, I'm a little less hesitant. On Tue, Nov 24, 2015, 13:33 Martín Acosta notifications@github.com wrote:
|
And could we relocate the discussion back to #651? This branch hasn't seen a commit in over 3 months, and Mithril's changed quite a bit in that time frame. |
@IMPinball yes, you right.
|
@IMPinball I think your confidence might be misplaced. incremental-dom is OK to use very recent tooling optimized for narrow assumptions because it was designed to be consumed by other tooling interfaces, to which it can afford to dictate concerns - which are very different to those of a package destined for direct author consumption in web applications which can be expected to have any number of esoteric requirements WRT packaging, build, and other deps. It's a bit like comparing the build options of a Babel plugin with those of jQuery. |
Good point. IMHO the easiest way to modularize would probably be to create On Wed, Nov 25, 2015, 06:11 Barney Carroll notifications@github.com wrote:
|
My vote is far, far and away for a simple concatenation of plain JS and no magical tool chain. I do want to preserve my ability to contribute to Mithril, and I doggedly refuse the complex and unnecessary 10 million dependency tool chains that others seem to love so much. Equally as much, if Mithril goes south or off and away to becoming a swiss-army-knife framework that does everything, and nothing very well, I can simply go on with whatever was the last version that made sense. The tight focus and tiny footprint is the primary reason I chose to build on top of Mithril; heck it does a thousand times more for me than moment.js at a fraction of the size. That means I don't want to inherit a thumping great big complicated tool-chain with Node.js and a gazillion dependencies, or a refactor-it-back-to-just-JavaScript job in the development side of Mithril. My $1.99 (inflation). |
@lawrence-dol agree!!!! let's keep it simple. |
Again, if you're not actively contributing this should make no difference whatsoever to the goodness you know and love in current Mithril. @IMPinball you may be interested in a 'best of all worlds approach' like what Andrea Giammarchi uses: https://github.com/WebReflection/gitstrap You can see this in practice on eg notify-js PS: Good to see you're still in the Mithril game @lawrence-dol :) |
AFAICT, all rollup does is resolving the It also supports circular dependencies, which means that Mithril could be split as is without decoupling anything. ES6 modules export and import bindings, not values like CommonJS modules do. //------ lib.js ------
export let foo = 3;
export function incFoo() {
foo++;
} //------ main.js ------
import { foo, incFoo } from './lib';
// The imported value is live
console.log(foo); // 3
incFoo();
console.log(foo); // 4
// The imported name can't be rebound from here
// (as if const here, even though it's mutable from its own scope).
foo = 9; // RollupError: Illegal reassignment to import 'odd'
foo++; // idem Beside maybe the function definition order, the resulting build artefact would be identical to the current mithril.js (hopefully appeasing @lawrence-dol). Edit: linked to a circular deps example; edit2: fixed the link which was badly urlencoded. |
@barneycarroll I think you meant this for notify-js... 😄 But anyways, something like that was what I was actually thinking of. V8 actually uses a similar system themselves, although it does this at runtime during initialization. I wouldn't use |
Could we all continue this discussion in #651? This PR is pretty much dead, and the current discussion is more on-topic there. |
This implements the proposal in #651.
Summary
This is a fairly extensive PR, modifying Mithril to use a package ecosystem. CommonJS-style (for node compatibility) packages together with Webpack (smallest footprint) are used.
Structure
The following folder structure is used:
core
includes packages that are used across all modules.Object.assign
polyfill is used to combine modules inindex.js
, which is the entry point. More entry points can be configured in webpack's configuration and built/tested individually.Benefits
mithril.js
, mithril.DOM.js,
mithril.router.js` etc.var m = require('mithril/DOM')
once a build for npm is set up.Changes List
In addition to the restructuring, the following changes to the code base were made:
several functions have been extracted to their own modules, and as such, calling them requires namespacing (
fns.propify()
etc.).I've added this line to
redraw
:$
is whereinitialize
is defined, and when we require it$document
becomes$.document
. The above functions can't be called like that it would seem, requiring the assignment. Not sure if this is the best solution.Tests for
console.log
anddocument.write
presence have been removed since we don't have a global IIFE anymore. Another method can be used.Gruntfile.js
was changed to accommodate webpack.for testing,
m.deps
is now inmock.js
.index.js
has this line:and in
tests/mithril-tests.js
:Since the tests and the script's output object are no longer in the same scope.
The cost
Is a few more milliseconds of loading and evaluating the script, as well as ~2kb of minified script size.