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

Minor fixes #2

Merged
merged 4 commits into from
Jul 7, 2016
Merged

Minor fixes #2

merged 4 commits into from
Jul 7, 2016

Conversation

zenekron
Copy link
Contributor

@zenekron zenekron commented Jul 7, 2016

Dependencies

  • removed benchmark from dependencies in favor of devDependencies
  • added microtime for better benchmark reports
  • added acorn and esprima as optionalDependencies

Benchmarks

Everything related to benchmarking has been moved to a bench folder. The original speed.js has undergone some refactoring and can now be found at bench/index.js. The output of the benchmarks is now stored in a results object which can be easily manipulated to extract desired data.

Default reporter

Loading sources in memory..
Starting benchmarks..
  esprima... done!
  acorn... done!
  lube... done!
+-------------------------------------------------+
|                     esprima                     |
+-------------------------------------------------+
|        filename        |     ops/sec     | runs |
+-------------------------------------------------+
|    angular-1.2.5.js    |    9.24  ±8.93% |  28  |
|    backbone-1.1.0.js   |   91.94  ±5.48% |  60  |
|      benchmark.js      |   62.18  ±4.36% |  46  |
|     jquery-1.9.1.js    |   16.97  ±6.27% |  31  |
| jquery.mobile-1.4.2.js |    9.21  ±8.23% |  27  |
|    mootools-1.4.5.js   |   19.39  ±7.27% |  35  |
|   underscore-1.5.2.js  |   89.87  ±4.06% |  52  |
|      yui-3.12.0.js     |   20.98  ±6.70% |  38  |
+-------------------------------------------------+
+-------------------------------------------------+
|                      acorn                      |
+-------------------------------------------------+
|        filename        |     ops/sec     | runs |
+-------------------------------------------------+
|    angular-1.2.5.js    |   11.91  ±6.85% |  32  |
|    backbone-1.1.0.js   |   83.53  ±4.73% |  49  |
|      benchmark.js      |   55.87  ±5.25% |  57  |
|     jquery-1.9.1.js    |   14.48  ±5.94% |  39  |
| jquery.mobile-1.4.2.js |    9.54  ±7.62% |  28  |
|    mootools-1.4.5.js   |   26.88  ±5.65% |  50  |
|   underscore-1.5.2.js  |  173.77  ±1.90% |  76  |
|      yui-3.12.0.js     |   42.22  ±1.10% |  54  |
+-------------------------------------------------+
+-------------------------------------------------+
|                       lube                      |
+-------------------------------------------------+
|        filename        |     ops/sec     | runs |
+-------------------------------------------------+
|    angular-1.2.5.js    |   27.17  ±7.93% |  42  |
|    backbone-1.1.0.js   |  250.93  ±4.85% |  66  |
|      benchmark.js      |  158.13  ±5.28% |  66  |
|     jquery-1.9.1.js    |   41.18  ±6.79% |  53  |
| jquery.mobile-1.4.2.js |   25.34  ±7.96% |  44  |
|    mootools-1.4.5.js   |   50.22  ±7.35% |  53  |
|   underscore-1.5.2.js  |  224.69  ±5.88% |  65  |
|      yui-3.12.0.js     |   57.69  ±6.35% |  50  |
+-------------------------------------------------+

Notes

With the resolution of #5900, engine.nodes should be changed again to * or whatever version of the engine this package should support.
For testing some users suggested the use of tape. I tried myself a mocha + chai + istanbul combo and they work pretty well together and help keeping the codebase sane.

I really suggest you to split lube.js in different files to improve readability and maintainability.

Benchmark is a library used for benchmarking the performance of scripts
and tools, and it wouldn't make much sense to have the final user
install it along with the module.
Esprima and/or acorn can be optionally installed to have a complete overview of the tools' speed.
Moved `sources/` to `bench/sources/`, also renamed `speed.js` to `bench/index.js`. This way everything that is related to benchmarking is located in a separate folder and benches can be run with `node bench` (or `npm run bench`).

The bench runner (`speed.js`) has undergone some major refactoring. While keeping the same features, modularity and result reporting have been improved.
Everything starting line #84 is completely optional.
@icefapper icefapper merged commit ae03ab6 into JazzleWare:master Jul 7, 2016
@icefapper
Copy link
Member

icefapper commented Jul 7, 2016

@zenekron
Thanks, I mean, this awesome!

really suggest you to split lube.js in different files to improve readability and maintainability.

Hmm; I've been considering this for some time now, and I totally agree with you the readability will improve if I do so, but I'm also very eager to keep it "browser-friendly", and I have fears splitting it into several smaller pieces might be against that goal. I'd be very glad to know your opinions on this; I'd also be more than glad to give you push access immediately if you find it appropriate.

@zenekron
Copy link
Contributor Author

zenekron commented Jul 8, 2016

@icefapper
You could virtually use any kind of preprocessor if well configured. You could even make a buildscript that uses lube to generate it's own AST and resolve the require statements from there.

Anyhow good solutions would be browserify to prepare a browser version and uglify to minify it.

Before messing with any of those you should refactor the whole code to be modular.

Namespaces

In plain javascript, to define a "namespace" you would do something like:

;(function(import1, import2, import3, exports) {
    exports.test = function() { ... }
    ....
})(lib1, lib2, lib3, myObject);

This works and is perfectly fine, but in Node there is a better way to handle them:

  1. Imports are handled via the require statement.
  2. Exports are defined at module.exports.
  3. Everything that's not exposed on the module.exports is not visible outside of the module (module scoping basically).

Keeping in mind these three points, the same code as above would look like:

var import1 = require('lib1');
var import2 = require('lib2');
var import3 = require('lib3');

function test() { ...}

module.exports = {
    test: test,
}

I'm saying this because in order to have modules behave correctly, you have to adopt this second syntax, which is not what you did in lube.js where you wrote _exports.Parser = Parser where _exports is the global namespace.

You really should focus on writing lube to run correctly on node, then let tools like browserify produce a browser build.

@zenekron zenekron deleted the minor-fixes branch July 8, 2016 06:59
@icefapper
Copy link
Member

icefapper commented Jul 9, 2016

@zenekron Thanks!
I have now broken the monolithic source into considerably smaller pieces, to be found at this location

Also, I wrote a dirty (as always) build script for sewing that abominable mess back together :)

Once again, I would be glad to know if I'm allowed to give you push access.

Thank for all the help.

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.

2 participants