Conversation
e50e2bd to
ff509b1
Compare
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| var cloneDeep = require('lodash').cloneDeep; |
There was a problem hiding this comment.
lodash has a lot of useful utility functions, might make more sense to require it on its own so that someone can use other lodash functions without changing this line.
There was a problem hiding this comment.
another note on lodash, we had previous goals to shrink bundle size, would this be going against that,
but CI being more critical, another option would be: we are using traverse.clone() in datasource-juggler
There was a problem hiding this comment.
lodash has a lot of useful utility functions, might make more sense to require it on its own so that someone can use other lodash functions without changing this line.
I am just following the style seen in existing code, see e.g.
Line 7 in 296c385
another note on lodash, we had previous goals to shrink bundle size, would this be going against that,
Bundle size is important, thank you for raising this concern. The browser bundle contains only the source code from lib/executor.js, config loader is executed during compilation, therefore it's ok to use lodash here.
There was a problem hiding this comment.
I stand by what I said but maybe that's just my preference.
It shouldn't stop this PR from being landed though.
|
LGTM |
|
LGTM2 |
|
Thank you for the review, I have landed the patch. |
This should fix the CI failures @loay is observing in strongloop/loopback-workspace#294 and myself in strongloop/loopback-workspace#298
@raymondfeng please review
cc @davidcheung @0candy @gunjpan @richardpringle