#13: lodash and classnames should be deps, not devDeps (closes #13) #14
Conversation
examples/webpack.config.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| var path = require('path'); | |||
| var webpack = require('webpack'); | |||
| var webpackBase = require('./webpack.base'); | |||
| var assign = require('lodash/object').assign; | |||
| var assign = require('lodash.assign'); | |||
There was a problem hiding this comment.
do you even need this with node 6?
There was a problem hiding this comment.
rephrased: can't you just use Object.assign?
There was a problem hiding this comment.
I don't need neither, I'm already refactoring these files in pure ES6 in a different branch (so ... instead of assign) :)
There was a problem hiding this comment.
mmm I think I'll do in this PR... a bit dirty but it's ok
package.json
Outdated
| "dependencies": {}, | ||
| "dependencies": { | ||
| "classnames": "^2.2.5", | ||
| "lodash.omit": "^4.5.0" |
There was a problem hiding this comment.
I know these dependencies are tiny, but you can probably remove both with
- const props = omit(this.props, ['children', 'ready', 'firstLaunchOnly', 'type']);
+ const { children, ready, firstLaunchOnly, type, ...props } = this.props;and simply joining strings instead of using classnames (which doesn't seem to be used in any interesting way)
There was a problem hiding this comment.
No dependencies is better than 2 dependencies 🎉
There was a problem hiding this comment.
yeah, why not 👍
There was a problem hiding this comment.
const { children, ready, firstLaunchOnly, type, ...props } = this.props;actually this is not ok as eslint would complain... I'll keep omit for now (as we're doing for every other component for this same stupid reason...)
There was a problem hiding this comment.
Oh, right. Well, we may consider https://www.npmjs.com/package/eslint-plugin-no-unused-vars-rest, but that's probably out of scope...
Closes #13
Test Plan
tests performed
tests not performed (domain coverage)