replace lodash custom lib by npm lodash

[lbineau]

The are still 2 problems with tests if you can help me with:

1) fulltext checks search: TypeError: Cannot read property 'name' of undefined at Context.test (tests/fulltextSpec.js:26:44)

2) itemjs tests with movies fixture makes search: TypeError: _chain(...).map is not a function at Context.test (tests/movies-sortSpec.js:54:48)

[cigolpl]

@lbineau thanks for trying. I'll try to test it out tomorrow. From initial looking - we should avoid package-lock.json and use only package.json if there is no specific reason

[lbineau]

It is just a normal file automatically generated by npm 5+ so everyone uses the same version of librairies (same kind of file than yarn.lock if you are familiar with). But sure no problem, we can remove it if you don't want to :-)

[cigolpl]

Actually I was not familiar with package-lock.json. I hope package.json with dependencies should keep the same dependency tree properly and should be enough. I am fan of "less is more" and "keep it simple as stupid" and I think we should keep this lib as small and simple as possible (even removing lodash from the codebase in the long term) but if you have good reasons and use cases to keep package-lock.json then we will keep it ;)

[cigolpl]

hello @lbineau, I've tested this PR and got the same error as you.. I don't know how to fix it in simple way. I don't see right now so much benefit of refactoring var _ = require('lodash'); into specific function like var _map = require('lodash/map');.

Imo it is better to provide small changes which are easier to merge and also some description in PR what kind of problem it solves (i.e. reduce code size, improve performance, adding new feature, etc) so then it's more clear for everyone what is the goal.

Thanks for this and I will update contribution page soon.

[lbineau]

For me the problem is when you import your library into another project that also uses lodash. Lodash is duplicated because you don't use npm. I understand that you did it to compile only the functions you need in lodash for file size purpose. So I propose to import only the function you need from lodash with npm. This way you keep the file size low and all the benefits of using npm.

Point 1 solved, I tried to comment better my latest commit. I've been mistaken the use of chain function. Now with flow function it works without importing the whole lodash lib. Point 2 I still don't know what causes the problem here.

[lbineau]

@cigolpl I don't contribute a lot to others' projects so that's why I not really aware the best way to contribute but I will take any advice you give me to do so :-)

[cigolpl]

@lbineau we are both learning ;) Description sounds good! +1 for flow - I didn't know it before

[cigolpl]

The code looks good to me. Once we resolve point 2 we should also make sure the file size of dist/itemsjs.js is not larger or similar than before with npm run browserify. Also we should consider using some lib for code minification in the future (maybe uglifyjs)

[lbineau]

I ran npm run browserify but the file is not minified, what is your current method to do that and be able to compare the size?

[lbineau]

By the way I found my stupid mistake I rename map instead of mapKeys.

[cigolpl]

That's the good question. There is no yet a method for minification here. I was trying today with browserify index.js -v -s itemsjs -t babelify | uglifyjs > dist/itemsjs.js and also with browserify index.js -v -s itemsjs | uglifyjs > dist/itemsjs.js but getting error:

Parse error at 0:2137,22
  _.map(items, (doc) => {
                      ^
ERROR: Unexpected token: operator (>)
    at JS_Parse_Error.get (eval at <anonymous> (/usr/lib/node_modules/uglify-js/tools/node.js:21:1), <anonymous>:79:23)
    at fatal (/usr/lib/node_modules/uglify-js/bin/uglifyjs:268:52)
    at run (/usr/lib/node_modules/uglify-js/bin/uglifyjs:225:9)
    at Socket.<anonymous> (/usr/lib/node_modules/uglify-js/bin/uglifyjs:163:9)
    at emitNone (events.js:91:20)
    at Socket.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickCallback (internal/process/next_tick.js:104:9)

[lbineau]

I think uglify isn't compatible with ES6 (yet) to minify the code of your lib in my project I used https://github.com/babel/minify It is basically babel (ES6 to ES5) + minification

[cigolpl]

@lbineau thanks for hint with https://github.com/babel/minify! I've added it to master and now npm run dist is creating dist/itemsjs.min.js file. It's working very well

[lbineau]

After some tests my version is heavier (58.2 KB vs 43.3 KB) but it drops with gzip (16.6KB vs 14.6KB) I wonder if the difference is the latest version of lodash that is heavier. Can you try to compile your libs/lodash.js with the latest version of lodash?

[cigolpl]

I think the difference is in:

lodash include=some,forEach,map,mapKeys,mapValues,every,includes,intersection,filter,keys,clone,flatten,transform,sortBy,orderBy -o lib/lodash.js -p

is more optimized than lodash minified with babel-minify and browserify. I think the second one is duplicating base code for many lodash methods. I had already latest version of lodash 4.17.4

[cigolpl]

@lbineau The concept of putting lodash to npm and using specific methods is good but it might be very difficult to make output optimized by size

[lbineau]

Yeah you are probably right. Anyway I will still use my fork for my project because I already use lodash with npm. Feel free to reject the PR, thanks for the time you spent on it! :-)

[cigolpl]

Cool, at least we have introduced some new things like minification