Bad module design

[ibc]

Bad:

• Usage of "Grunt concat" task to generate a browser-suitable library.
• Setting "main": "./dist/htmlminifier.js" in package.json.

How should it be?:

• Create the whole code as if it was a pure Node project, so "main": "index.js" in package.json, and in "index.js" call to require('lib/htmlparser'), etc.
• Use browserify (via a Grunt task) to generate a bundle file suitable for the browser, and store it in dist/htmlminifier.js.
• Then in package.json: "browser": "dist/htmlminifier.js".

[feross]

You don't need the last bullet point:

• "Then in package.json: "browser": "dist/htmlminifier.js"."

Browserify users who do require('htmlminifier') will have no problems, as browserify will just browserify the file. The dist/htmlminifier.js is just for users who use bower, component, or just want a prebuilt script to use.

[ibc]

Yes, sorry, you are right.
Thanks.

[feross]

No problem :)

[kangax]

Thanks folks, PR is welcome!

Sent from my iPad

On Nov 10, 2014, at 2:17 AM, Iñaki Baz Castillo notifications@github.com wrote:

Bad:

Usage of "Grunt concat" task to generate a browser-suitable library.
Setting "main": "./dist/htmlminifier.js" in package.json
How should it be?:

Create the whole code as if it was a pure Node project, so "main": "index.js" in package.json, and in "index.js" call to require('lib/htmlparser'), etc.
Use browserify (via a Grunt task) to generate a bundle file suitable for the browser, and store it in dist/htmlminifier.js.
Then in package.json: "browser": "dist/htmlminifier.js".
—
Reply to this email directly or view it on GitHub.

[jessehattabaugh]

What's the plan here? If we change package.json to {main: 'src/htmlminifier.js'} and leave grunt->concat->dist/htmlminifier.js alone will that fix Browserify, or break any other package systems? Or does src/htmlminifier.js need to be changed to support Browserify to replace concat? I'm kinda stuck here.

[ibc]

IMHO the design should be:

• Create the project as a pure Node project (without concat at all).
• Test the project (test units) with nodeunit or similar (this is, test the Node project with all its files/modules, rather than testing the "browser" library version).
• browserify the project with --standalone NAME_TO_EXPORT_TO_WINDOW.

Period. But honestly, a good library cannot rely on concatenating files into a single one.

[kangax]

Create the project as a pure Node project (without concat at all).

What's wrong with concat?

Usage of "Grunt concat" task to generate a browser-suitable library.

Why is it bad specifically for "browser-suitable" library? And what do you mean by "browser-suitable"?

[ibc]

What's wrong with concat?

Building a library with "concat" is like doing copy&paste. Those files have no sense by them alone. Even worse: most probably they are syntactically incorrect, so you cannot process them with JSHint/JSLint.

Concatenating files is not the way to do nothing in JavaScript nowadays. Again: code it as if it was a pure Node project, make your project "export" something (via module.exports) and then browserify the whole code with browserify.

Why is it bad specifically for "browser-suitable" library? And what do you mean by "browser-suitable"?

A Node project is composed of external modules (libraries) and internal modules (which in fact are other files under the lib/ folder). All those "dependencies" (including both external and internal modules) are loaded using require() (similar to require in Ruby or import in Python). A Node project "exports" something. In fact each Node library and each internal file under a Node project lib folder export something. This is achieved by settings module.exports.

After the Node project is done, a Grunt o gulp task can easily run browserify on it to generate a single JavaScrip file which contains all the dependencies in it. This is NOT a simple "concat" mechanism at all. In fact, the resulting "bundle" still keeps all the calls to the function require() and module.exports.

require() and module.exports do not exist in vanilla JavaScript (they are defined in CommonJS and will exist in ECMAScript6), but browserify defines them at the top of the bundle (the browserified file "suitable for a browser").

"Suitable for a browser" means that you can use it as usual within your HTML:

<script src="js/html-minifier.bundle.js"></script>

Take a look to this project in which I'm co-author: JsSIP.
Months ago it used concat which was unmaintainable. Now it is a pure Node project and includes a grunt-browserify task to build the "browserified" version, which you can find on the builds/ folder:

• https://github.com/versatica/JsSIP/tree/master/builds

For a simpler example, take a look to this other project: https://github.com/ibc/args2object (but it is not a good example since its source file is a single JS file, but let's imagine there are more files):

• If I want to use such a args2object library in my Node project I do:

$ npm install args2object

// in my Node files:
var args2object =  require('args2object');

This is, I do NOT need to do require for a "concatenaded" file. It is better to use the module as a pure Node module. Let's just use the browserified file for those who code web applications and load the JavaScript files in the old way, this is:

<script src="js/A.js"></script>
<script src="js/B.js"></script>
<script src="js/D.js"></script>
<script src="js/C.js"></script>

(oppss, error because D.js depends on a global window.C object defined by C.js (which is loaded after D.js...).

I've learnt all this stuff about Node (and building browser suitable libraries from a Node project) recently, and I cannot live without it now.I strongly think this is the way to go today.

For example, all the stuff you do at https://github.com/kangax/fabric.js/blob/master/dist/fabric.require.js is much easier (and robust) if you code as I suggest above and them use browserify to build such a bundle.

[jessehattabaugh]

I don't care whether or not you use concat.

What I care about is that when I require("html-minifier") and bundle my code with Browserify, it reads in your concatted ./dist/htmlminifier.js file because package.json says main: ./dist/htmlminifier.js, but when it gets to line 580 of ./dist/htmlminifier.js Browserify throws an error because require is in fact a function and ./dist/htmlparser.js doesn't exist.

If you are publishing a package to NPM package.json "main" should point to a file that can be required as a CommonJS module. I believe there are other properties of package.json that can point to bundled code appropriate for direct inclusion on the client side. It's a fine idea to use Browserify to bundle ./dist/htmlparser.js because it would save you from having to check whether or not require is a function all the time. But for my purposes, all I care is that htmlparser.js exists when your code require()s it.

[ibc]

@kangax I've forked the project and will try to make a PR with a new "Node-based" design. However the fact that test units require "qunit" (which internally requires "pantomjs") annoys me.

Test units in this library are supposed to take a string, produce another string and match them with a expected string. No more. There is no need for having a browser emulation within it. Is there something wrong in using any other testunit library that does not depend on Phantomjs? Phantomjs is not relieable, it fails to load some JS files (for example all those generated with browserify), sometimes it fails to install, etc etc, and it is not needed at all for testing html-minifier. I will get rid of it in my fork and use nodeunit instead.