Change implementation to be modular #2

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@knpwrs
knpwrs commented Dec 30, 2012

This changes smokesignals to be modular in such a fashion that the same file can be loaded by CommonJS (node's require), AMD (RequireJS), or just included on a page.

@bentomas
Owner

Hey good idea! Unfortunately, I'm not going to make the file any bigger for the people directly including it. Goal number 2 is to be as small as possible, and this makes it quite a big bigger (by 40%). I've tried very hard to make it as small as I possibly can, and won't be accepting any changes to the minified version that either A) don't make it smaller, or B) don't fix bugs.

I would, however, consider allowing another two files to be added (in addition to smokesignals.js and smokesignals.min.js--maybe call them smokesignals.mod.js and smokesignals.mod.min.js) for people trying to include it with a loader of some sort. But the way you have done it isn't the most efficient size-wise. At the very least these lines from an older version of this library use fewer characters: https://github.com/bentomas/smokesignals/blob/c18e3f8f5aaaf996587db90413b4a1812359bf45/smokesignals.js#L1-L6
(I saw no reason to check for the amd property since for the module check in the previous section we cannot do any additional checking either).

If you want to code that up , I'd merge that! Maybe also remove all the comments from this new file and have a comment pointing them to the canonical version of the lib (smokesignals.js).

Thanks for being interested in making this library better and more versatile!

@bentomas bentomas closed this Dec 30, 2012
@knpwrs
knpwrs commented Dec 30, 2012

Just curious, if you previously supported AMD why did you remove it?

@bentomas
Owner
bentomas commented Feb 5, 2013

Woops! Somehow missed this comment last month!

To make the library considerably smaller. And no one complained.

Plus module loaders like that seem anathema to the philosophy of making libraries as small as possible. The require.js file is 14.6KB minified or 9.5 gzipped. That's a lot of page weight for something that can be fairly easily done with a simple <script src=""></script> and a precompiler. I'm not saying they aren't useful, I'm just saying I wasn't willing to sacrifice the goals of this project to support them.

But like I said, I'd be happy to add another file to the project which adds that functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment