Financial-Times / ft-origami

The Old Origami Website, do not use
https://github.com/Financial-Times/origami
76 stars 12 forks source link

Disallow non-semvery dependencies e.g. lodash-npm #401

Closed wheresrhys closed 8 years ago

wheresrhys commented 9 years ago

Lodash is being used in a handful of places where a microlibrary/ES6 array methods would be better. e.g in a few places its debounce fumctionality is used, where perhaps something like https://github.com/niksy/throttle-debounce should be advocated.

AlbertoElias commented 9 years ago

But we only use that small bit of the module, not the whole thing

wheresrhys commented 9 years ago

Yes, but it still feels untidy. And I think lodash/debounce brings in a load of other files making the total amount of code larger than needs be for a simple debounce function e.g. it imports

var isObject = require('../lang/isObject'),
    now = require('../date/now');

... which in turn import other files

triblondon commented 9 years ago

We don't see a problem with components using lodash. It's well tested, trusted to be high quality, and likely to on average be better than random microlibs from npm. However, where there are native array methods available, we should definitely use them, that's already advised in the spec.

wheresrhys commented 9 years ago

Sorry for reopening, but I just noticed that another problem with lodash is that if you want to use submodules such as lodash/function/debounce then you have to use the -npm package, which isn't published using valid semver e.g. 3.10.1-npm, and this leads inextricably to the code smell of bower resolutions (as bad a smell as !important in css IMHO).

The original issue went a bit far in discouraging use of lodash. But I think packages that don't use semver should be discouraged (which would still allow for lodash to be used, but only in its built form (which is published using semver)). As origami only uses it for debounce I think lodash-npm's non-semveriness is a good reason to move to using an actual microlibrary instead.

triblondon commented 8 years ago

@wheresrhys we remain comfortable with lodash (the full version) being a dependency of modules, even if they don't use much of it, because it's small and our build process guarantees that we'll only have one copy in the bundle. However, we're also happy to accept PRs on individual components to replace lodash with a microlib (or if it's just debounce, to be honest probably easier just to paste an implementation directly into the source of the module).