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

lodash is redundant #95

Closed
hdf opened this issue Apr 21, 2015 · 13 comments
Closed

lodash is redundant #95

hdf opened this issue Apr 21, 2015 · 13 comments

Comments

@hdf
Copy link

hdf commented Apr 21, 2015

lodash is required by both helmet-csp and hsts. It is a fairly big dependency, that is installed twice. If it was set as a direct helmet requirement, than it would only be installed once (theoretically). Currently helmet is larger than express because of this, which is ridiculous.

@EvanHahn
Copy link
Member

What if we installed the individual lodash dependencies? So instead of this:

var _ = require('lodash')

_.isArray(foo)
_.isObject(bar)

...we'd do this:

var isArray = require('lodash.isarray')
var isObject = require('lodash.isobject')

isArray(foo)
isObject(bar)

Thanks for pointing this out!

@hdf
Copy link
Author

hdf commented Apr 21, 2015

This has nothing to do with what I said.

@trygve-lie
Copy link

I think we can remove lodash. Looking through the code it seems to me that lodash are only used for isString(), isFunction() etc and a Object deep copy and one Array mapping.

The Object deep copy are only used once. That can be pulled into a separate function in Helmet instead. There is also one usage of .map which can be replaced by the native ES5 .map method.

For the isString(), isFunction() etc we can in node.js 0.12 and iojs 1.x use the equivalent in the native util module. Though, these functions are not in node.js 0.10 but are provided as a userland module for 0.10 and older versions of node.js: core-util-is.

My suggestion is to use the core-util-is module for the "is" checks and keep a inhouse version of the Object deep copy and remove lodash. If this sounds OK I'll gladly provide a PR for these changes.

@hdf
Copy link
Author

hdf commented Apr 21, 2015

Well, if lodash could be eliminated completely, that would be great, as it is a massive thing, in itself is almost as large as express.

@trygve-lie
Copy link

Talking about size; I think its also possible to remove the dependency to connect (and still support connect).

@hdf
Copy link
Author

hdf commented Apr 21, 2015

Express 4 no longer uses Connect, why are we?

@EvanHahn
Copy link
Member

I don't think we should write things like isFunction or object copying ourselves. Lodash is battle-tested and has thought about weird edge cases that I don't think we should think about.

I agree that requiring all of Lodash is probably overkill. We can just require the bits and pieces that we need. While that's not the same as "only include Lodash once," I think it solves the underlying problem.

As far as Connect goes, we don't need it. We just need something that can "concatenate" a bunch of middlewares together. We could likely use something like Async's compose or something similar. In any case, I think this is a separate issue—I made #97 for this.

What do you think?

@trygve-lie
Copy link

The above PR use the same isFunction checks as node.js / iojs use themself internally. Tons of modules rely on these checks and I would say that these checks are pretty battle-tested also.

Yes; they are a tiny bit simpler than the lodash checks but are the very tiny amount of corner cases really an issue? I don't think so.

Agree that the connect question are a separate issue.

@trygve-lie
Copy link

Looking at the deep copy in csp; is a deep copy really needed? From what I can see, the Object being cloned is quite flat.

@EvanHahn
Copy link
Member

Closing this in favor of helmetjs/csp#12.

@trygve-lie
Copy link

This removes Lodash from CSP also: helmetjs/csp#13

@EvanHahn
Copy link
Member

Thanks @hdf for bringing this up and @trygve-lie for the PRs! I released helmet@0.8.0, hsts@0.1.2 and helmet-csp@0.2.3 which removes the Lodash dependencies (and fixes a few other things).

@trygve-lie
Copy link

Thanks for pulling the PRs and releasing this fast :-)

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

No branches or pull requests

3 participants