You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2024/01/22 04:54:44 UTC
[PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
rusackas opened a new pull request, #26710:
URL: https://github.com/apache/superset/pull/26710
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
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
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904491958
> I can revise this PR to do module imports if this pattern bothers people
Do you mean `member` imports? The reason I'm asking is because we don't need the whole module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904439491
@rusackas Is it possible to replace the occurrences of `_` with a named import `{ map }` and use `member` instead of `method`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26710:
URL: https://github.com/apache/superset/pull/26710#discussion_r1463236273
##########
superset-frontend/packages/generator-superset/generators/plugin-chart/index.js:
##########
@@ -20,7 +20,14 @@
/* eslint-disable sort-keys */
const Generator = require('yeoman-generator');
-const _ = require('lodash');
Review Comment:
Can't we do `const { kebabCase, startCase, camelCase, upperFirst } = require('lodash');` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904483204
@rusackas Is there any chance that that eslint plugin could flag only default imports from `"lodash"`, but not default imports from `"lodash/feature_name"` and named imports from `"lodash"`? I think that would be perfect for us
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1903533970
I see that some files are using `import _ from 'lodash';` which is definitely not treeshakable. Maybe it'd be enough to fix just those few cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #26710:
URL: https://github.com/apache/superset/pull/26710#discussion_r1463904470
##########
superset-frontend/packages/generator-superset/generators/plugin-chart/index.js:
##########
@@ -20,7 +20,14 @@
/* eslint-disable sort-keys */
const Generator = require('yeoman-generator');
-const _ = require('lodash');
Review Comment:
Just saw this after merging..... but... yes I suppose we can. I'll open a smaller/less contentious PR on some rainy day
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904418000
It's always a slippery slope...
Indeed, there were a few places importing `_` which loads the whole darn thing. I fixed those, and this was a nice, small PR.
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 `module` and `method` imports... it doesn't let you mix and match.
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1905042061
OK, everything is now converted to `member` rather than `method` :) Let's hope it all builds correctly now, and our bundles get smaller!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904464787
I think the main reason for using named imports/exports is because is the default syntax in Superset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904492240
> 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)
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1903255883
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26710?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`d34874c`)](https://app.codecov.io/gh/apache/superset/commit/d34874cf2bbd6385d2ca6f38856d3d10d6fd745b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.48% compared to head [(`ac1c25e`)](https://app.codecov.io/gh/apache/superset/pull/26710?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 55.54%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26710 +/- ##
===========================================
- Coverage 69.48% 55.54% -13.94%
===========================================
Files 1894 1894
Lines 74138 74107 -31
Branches 8243 8242 -1
===========================================
- Hits 51514 41163 -10351
- Misses 20555 30876 +10321
+ Partials 2069 2068 -1
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.85% <ø> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.80% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `54.12% <ø> (-28.89%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/26710/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/26710?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1903698361
+1 to @kgabryje's comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904449923
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 `_` is trivial either way, but going with `method` seems to have more going for it, other than taking up multiple lines. if everyone wants module, I'll go the other direction, but the PR will still be a big one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1904487990
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #26710:
URL: https://github.com/apache/superset/pull/26710
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dependencies): stopping (and preventing) full lodash library import... now using only method level imports. [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #26710:
URL: https://github.com/apache/superset/pull/26710#issuecomment-1903530964
According to [lodash docs](https://lodash.com/per-method-packages), [babel-plugin-lodash](https://github.com/lodash/babel-plugin-lodash) (which we are using) makes imports like `import { x } from "lodash"` tree-shakable just like `import x from lodash/x`. Do you know which module appears to import the whole lodash without tree shaking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org