fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. #26710
Conversation
rusackas
requested review from
michael-s-molina,
geido and
kgabryje
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #26710 +/- ##
=======================================
Coverage 69.48% 69.48%
=======================================
Files 1894 1894
Lines 74151 74151
Branches 8243 8243
=======================================
Hits 51527 51527
Misses 20555 20555
Partials 2069 2069
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
According to lodash docs, babel-plugin-lodash (which we are using) makes imports like |
|
I see that some files are using |
|
+1 to @kgabryje's comments. |
|
It's always a slippery slope... Indeed, there were a few places importing Then, I thought "gee, we should prevent this from happening, is there a lint rule?" and there is! So installed it, and found a catch... while you can block the full import, it forces consistency between I chose method because (a) it was a simpler, smaller migration, and (b) this produces a reliably small bundle, even in an environment where we might not trust our tree-shaking fully (and I'm not sure if I do). |
|
@rusackas Is it possible to replace the occurrences of |
|
Yes, it's possible... we already have 36 method-style imports in Superset, so we have to force consistency one way or the other. Method is the default/preference of the lodash community, and is faster to compile since no treeshaking is required, and provides no opportunity for surprises. Replacing the instances of |
|
I think the main reason for using named imports/exports is because is the default syntax in Superset. |
|
@rusackas Is there any chance that that eslint plugin could flag only default imports from |
|
Nope, it doesn't support that. I can revise this PR to do module imports if this pattern bothers people, or close this one and open a new PR... whichever is easier. It's not a priority, so that would happen on some other rainy day (I opened this on a Sunday night on a whim) |
Do you mean member imports? The reason I'm asking is because we don't need the whole module. |
Oh that's a shame... In such case, I think it's more important to prevent default imports from lodash than to stick to named imports style - especially since if we let just 1 bad import slip past code reviews, it will include the whole library in our bundle. |
This reverts commit ac1c25e.
|
OK, everything is now converted to |
| @@ -20,7 +20,14 @@ | |||
| /* eslint-disable sort-keys */ | |||
|
|
|||
| const Generator = require('yeoman-generator'); | |||
| const _ = require('lodash'); | |||
There was a problem hiding this comment.
Can't we do const { kebabCase, startCase, camelCase, upperFirst } = require('lodash'); instead?
There was a problem hiding this comment.
Just saw this after merging..... but... yes I suppose we can. I'll open a smaller/less contentious PR on some rainy day
…rt... now using only method level imports. (#26710)
…rt... now using only method level imports. (apache#26710)
…rt... now using only method level imports. (apache#26710)
mistercrunch
added
🏷️ bot
…rt... now using only method level imports. (apache#26710)
SUMMARY
There were a few spots where the entire lodash library was being loaded, bloating our bundles for no good reason. Now there's an eslint rule to prevent this, and the code has been adjusted to use/allow only method level imports.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION