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

Making Mithril modular #665

Closed
wants to merge 6 commits into from
Closed

Making Mithril modular #665

wants to merge 6 commits into from

Conversation

srolel
Copy link

@srolel srolel commented Jun 12, 2015

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:

|-- mithril.js
    |-- src
        |-- index.js
        |-- core
        |   |-- fns.js
        |   |-- init.js
        |   |-- types.js
        |-- helpers
        |   |-- Object.assign.js
        |-- modules
            |-- DOM.js
            |-- http.js
            |-- mock.js
            |-- router.js
            |-- utils.js

core includes packages that are used across all modules.
Object.assign polyfill is used to combine modules in index.js, which is the entry point. More entry points can be configured in webpack's configuration and built/tested individually.

Benefits

  1. Mithril as a library can now import modules to for usage within the library.
  2. Specialized builds of Mithril can be made.
    • A separate file for each built can be made, for example: mithril.js, mithril.DOM.js,mithril.router.js` etc.
    • Module ecosystems can use var m = require('mithril/DOM') once a build for npm is set up.
  3. Modules allow for a more organized codebase which is easy to reason about, refactor, and add to.

Changes List

In addition to the restructuring, the following changes to the code base were made:

  1. several functions have been extracted to their own modules, and as such, calling them requires namespacing (fns.propify() etc.).

  2. I've added this line to redraw:

    var raf = $.requestAnimationFrame, caf = $.cancelAnimationFrame;
    

    $ is where initialize 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.

  3. Tests for console.log and document.write presence have been removed since we don't have a global IIFE anymore. Another method can be used.

  4. Gruntfile.js was changed to accommodate webpack.

  5. for testing, m.deps is now in mock.js. index.js has this line:

    if (typeof global != "undefined") global.m = m;
    

    and in tests/mithril-tests.js:

      var m = global.m;
    

    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.

@srolel srolel mentioned this pull request Jun 12, 2015
@dead-claudia
Copy link
Member

+1 for this.

@barneycarroll
Copy link
Member

👍 from me too. Apart from anything else, this makes the source far easier to reason about.

@venning
Copy link
Contributor

venning commented Jul 1, 2015

Relative sizes:

  • prior to modularization:
    • mithril.min.js 18,592 bytes
    • mithril.min.js.gz 7,364 bytes
  • after modularization:
    • mithril.min.js 21,216 bytes, which is +2,624 bytes or +14.1%
    • mithril.min.js.gz 8,155 bytes, which is +791 bytes or +10.7%

I used the commit immediately prior to this PR and I used gzip -9. The module boilerplate lends itself well to run-length encoding, so the compressed weight gain is lower than the uncompressed, relative to the original.


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];
Copy link
Member

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.

@dead-claudia
Copy link
Member

@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);
Copy link
Member

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.

@dead-claudia
Copy link
Member

Also, much of the modularization work includes some extra, and in a couple cases, unnecessary, boilerplate as well. A couple examples of this:

  1. (already noted in a comment) A complete Object.assign polyfill for a single use case that can be fulfilled with just the following code:
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 m.prop could work without Mithril deferreds, it already can work out of the box with ES2015 promises and Promise/A+ implementations. You literally don't need m.deferred to use it. If you want, use another deferred implementation. Makes no difference.

@srolel
Copy link
Author

srolel commented Jul 1, 2015

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

@venning
Copy link
Contributor

venning commented Jul 1, 2015

@mosho1, how are you building this? I haven't looked at it too closely, but it looks like the webpack:build task is what produces mithril.js. I get grunt erroring on me and no build even with --force.

@srolel
Copy link
Author

srolel commented Jul 1, 2015

@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

@venning
Copy link
Contributor

venning commented Jul 2, 2015

Thanks, @mosho1, I got it to build with that update, which also modified the outputted mithril.js (and thus mithril.min.js) slightly, so I have adjusted the numbers accordingly below.


@IMPinball, here are the results of your experiments:

minified ∆ bytes ∆ % gzipped ∆ bytes ∆ %
pre-modularization 18,592 7,364
full modularization 21,201 +2,609 +14.0% 8,152 +788 +10.7%
DOM only 13,667 -4,925 -26.5% 5,395 -1,969 -26.7%
w/ router, mock 17,109 -1,483 -8.0% 6,639 -725 -9.8%
w/ router, utils, mock 17,114 -1,478 -7.9% 6,624 -740 -10.0%
w/ utils, http, mock 18,284 -308 -1.7% 7,150 -214 -2.9%

I did not attempt any of the other tweaks, such as replacing the Object.assign polyfill.

@dead-claudia
Copy link
Member

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.)

@oren
Copy link

oren commented Jul 2, 2015

@IMPinball I wonder if it also going to be easier to incrementally migrate an Angular app to Mithril app.

@venning
Copy link
Contributor

venning commented Jul 2, 2015

I could make an argument for even more breakdown within DOM for very small projects that just need to be light and fast: remove m() and rely on template pre-compilation; remove componentization due to only having a few pieces; and add in only what else you need for your specific application, like http.

Also, it looks like some of core/fns is not actually "core": parseQueryString is only ever used in router; buildQueryString is used by both http and router. Right now, the DOM only build winds up with both parseQueryString and buildQueryString and so it's bigger than it needs to be.

@dead-claudia
Copy link
Member

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.)

@dead-claudia
Copy link
Member

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.

@srolel
Copy link
Author

srolel commented Jul 2, 2015

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:

  • prior to modularization:
    • mithril.min.js 18,592 bytes
    • mithril.min.js.gz 7,364 bytes
  • after modularization:
    • mithril.min.js 20,993 bytes, which is +2,404 bytes or +12.9%
    • mithril.min.js.gz 8,078 bytes, which is +714 bytes or +9.7%

DOM only is 12,927 minified and 5,093 gzipped.

@pelonpelon
Copy link
Contributor

This is really exciting. Is there a way to factor out Mithril deferred so it can be used sans http.js or m.request used with future ES2015 promises?

@venning
Copy link
Contributor

venning commented Jul 3, 2015

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 mithril.js is for developers, then perhaps better commenting is in order. lodash was mentioned in the original issue as an example of modular builds. Well, lodash.js has 4,333 lines of code and 7,292 lines of comments, and it all exists in one file. They still manage to build out a far more robust modularization strategy without the expense of a guaranteed webpack. Granted, anything similar to lodash-cli would be a heavy lift. With better commenting could come JSDoc annotations that would, perhaps, allow Closer Compiler to squeeze Mithril down in size further.

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 mithril.js as one file and using some method during development of producing subset Mithril "builds". You could cut them down based on any common use cases. If you need UMD compliance, additional builds could be wrapped up in a more automated way. This would keep Mithril's codebase tight and sane while accepting that there are divergent use cases. I will work on a proposal to do this.

Thoughts?

@srolel
Copy link
Author

srolel commented Jul 3, 2015

@pelonpelon That should be quite possible. Splitting things into modules is just a matter of refactoring, if at all. In this case extracting m.deferred doesn't look hard.

@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 dist folder with all the various build combinations: mithril.DOM.js, mithril.router.js etc., Similar to RxJS for example. I agree it's a leaner option that fits the spirit of Mithril better, and in Mithril's case perhaps webpack is overkill. That shouldn't be hard to implement at all either, I can probably get it done tomorrow.

@dead-claudia
Copy link
Member

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.

@barneycarroll
Copy link
Member

WRT experimental green field dev…

@lhorie we missed you :) this is great news — sounds awesome!

lightweight anti-pattern…

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 fetch polyfill. Likewise for routing it's perfectly valid to decide that some library dedicated to that purpose is more complete for my requirements or more attuned to my style, and opt not to include Mithril's module for that. For instance the fact that Mithril's totally legitimate and very elegant route modes don't interop well with URIjs is an issue adopters should be able to make an executive decision on without the unnecessary bloat and cognitive dissonance of having a codebase with different ways of doing the same thing.

@mneumann
Copy link

mneumann commented Jul 9, 2015

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

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.

@dead-claudia
Copy link
Member

@mneumann

If @lhorie can make Mithril faster than cito.js, then you have a win-win situation here. 😉

@mneumann
Copy link

mneumann commented Jul 9, 2015

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

@dead-claudia
Copy link
Member

@lhorie Can this be closed? I'm not recommending that #651 be closed, but before that proposal can be effectively implemented, there's a few other things that need to happen first before this becomes practical to do.

@kolya-ay
Copy link

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.

@dead-claudia
Copy link
Member

I already like that. It does full ES6 module resolution. How stable is it?
Can it process full ES6 + JSX? I might use it in my own projects, as I
currently use Babel and Browserify primarily, and with ES6 projects, that's
completely equivalent.

On Thu, Oct 29, 2015, 14:03 Kolya Ay notifications@github.com wrote:

It seems that rollup.js http://rollupjs.org/ should be used for
modularization instead of Webpack. It generates almost no boilerplate and allows
access https://github.com/rollup/rollup/wiki/jsnext:main Mithril parts
for powerusers while maintaining single core.


Reply to this email directly or view it on GitHub
#665 (comment).

@kolya-ay
Copy link

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

@suhaotian
Copy link

very good.

@pygy
Copy link
Member

pygy commented Nov 24, 2015

FWIW, rollup is used for building Google's incremental-dom.

@kolya-ay
Copy link

@pygy Seems like a one small step to enterprise admission;)

@veggiemonk
Copy link
Contributor

If anybody is interested, JSPM contains a ES6 module loader.
Does anybody already made an ES6 module version of mithril?

@barneycarroll
Copy link
Member

@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 import / export directives to CommonJS require invocations and exports assignments, which works fine with current Mithril.

@tinchoz49
Copy link

@dead-claudia
Copy link
Member

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:

exactly as @barneycarroll https://github.com/barneycarroll is saying...

https://github.com/geut/mithril-transition/tree/master/example


Reply to this email directly or view it on GitHub
#665 (comment).

@dead-claudia
Copy link
Member

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.

@dead-claudia
Copy link
Member

@mosho1 @lhorie Would either of you mind closing this PR? Even if we were to go this route, it would have to be rewritten from scratch, anyways.

@tinchoz49
Copy link

@IMPinball yes, you right.
One thing, I don't understand what is the relation between incremental-dom and my comment with the example using babel with browserify? Incremental-dom uses rollup.js

Given incremental-dom is using it, I'm a little less hesitant.

@barneycarroll
Copy link
Member

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

@dead-claudia
Copy link
Member

Good point. IMHO the easiest way to modularize would probably be to create
a concatenation script and provide different bundles. Or figure out how
Lodash handles it, and copy off them.

On Wed, Nov 25, 2015, 06:11 Barney Carroll notifications@github.com wrote:

@IMPinball https://github.com/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.


Reply to this email directly or view it on GitHub
#665 (comment).

@lawrence-dol
Copy link

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).

@veggiemonk
Copy link
Contributor

@lawrence-dol agree!!!! let's keep it simple.

@barneycarroll
Copy link
Member

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

@pygy
Copy link
Member

pygy commented Nov 25, 2015

AFAICT, all rollup does is resolving the import / export bindings in a way that's both optimal and human friendly (in that it preserves variable names, only appending $1 etc. on conflict), and everything is wrapped up in ( AMD | CommonJS | global IIFE | UMD | ES6 module ).

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.

@dead-claudia
Copy link
Member

@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 make, as I can tell some contributors do use Windows (I suspect @lhorie does, as the project has always used CRLF endings), but I was thinking of a similar process. I could write a little Recast tool that could take in IIFEs and export combined modules, with a config list. It would be easy to do with Grunt, to be honest. I could also do similar with Babel + Babylon, without using any of the ES2015 presets (so nothing would otherwise be transformed).

@dead-claudia
Copy link
Member

Could we all continue this discussion in #651? This PR is pretty much dead, and the current discussion is more on-topic there.

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

Successfully merging this pull request may close these issues.

None yet