Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove lodash.throttle and replace underscore calls with lodash #5946

Merged
merged 6 commits into from
Sep 27, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Sep 20, 2018

  • Remove lodash.throttle from dependency since there is now lodash.
  • Add babel-plugin-lodash that helps optimize bundle output by taking only necessary part from lodash instead of the entire bundle.
    https://github.com/lodash/babel-plugin-lodash
  • Replace underscore calls with lodash where applicable.

@williaster @xtinec @conglei

@kristw kristw force-pushed the kristw-throttle branch 2 times, most recently from aa56827 to 37d4d27 Compare September 21, 2018 17:51
@kristw kristw closed this Sep 21, 2018
@kristw kristw reopened this Sep 21, 2018
@kristw kristw closed this Sep 21, 2018
@kristw kristw reopened this Sep 21, 2018
@codecov-io
Copy link

Codecov Report

Merging #5946 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5946      +/-   ##
==========================================
- Coverage   63.58%   63.56%   -0.02%     
==========================================
  Files         393      393              
  Lines       23674    23661      -13     
  Branches     2639     2639              
==========================================
- Hits        15053    15041      -12     
+ Misses       8608     8607       -1     
  Partials       13       13
Impacted Files Coverage Δ
...t/assets/src/components/InfoTooltipWithTrigger.jsx 56.25% <ø> (-2.58%) ⬇️
superset/assets/src/visualizations/nvd3/NVD3Vis.js 7.76% <ø> (-0.23%) ⬇️
superset/assets/src/components/TooltipWrapper.jsx 100% <ø> (ø) ⬆️
...assets/src/dashboard/components/dnd/handleHover.js 44.44% <ø> (-5.56%) ⬇️
...uperset/assets/src/SqlLab/components/SqlEditor.jsx 77.66% <ø> (-0.22%) ⬇️
superset/assets/src/reduxUtils.js 75% <ø> (-0.41%) ⬇️
superset/assets/src/components/Button.jsx 90.47% <ø> (-0.44%) ⬇️
superset/assets/src/modules/utils.js 52.84% <ø> (-0.76%) ⬇️
superset/assets/src/components/AlteredSliceTag.jsx 100% <ø> (ø) ⬆️
...ssets/src/visualizations/deckgl/layers/polygon.jsx 0% <0%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c62494...48f683b. Read the comment docs.

@@ -10,12 +9,6 @@ import {
} from '../../../src/modules/utils';

describe('utils', () => {
it('slugify slugifies', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we still rewrite but keep this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested stubbing Superset's slugify implementation with kebabCase from lodash and it passes all test cases under this it except one.

kebabCase: "camelCase" => "camel-case"
slugify: "camelCase" => "camelcase"

This was the only difference. IMO, I am leaning towards switching to lodash's kebabCase which is well-tested library and industry standard than custom implementation and then remove slugify from codebase.

I checked where the function slugify is being used and it is used for creating tooltip id string. My understanding is this is harmless change, so I made the move for it. Please correct me if I misunderstood.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it used in creating customized url. for example /superset/dashboard/homesmetrics/.
can you make sure existed slugged dashboard url work?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think url is generated by python slugify function...so this JS version is just "tooltip". so it really didn't matter much.

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kristw
Copy link
Contributor Author

kristw commented Sep 26, 2018

Thank you. Please merge when you are ready.

@mistercrunch mistercrunch merged commit 4c21c65 into apache:master Sep 27, 2018
@kristw kristw deleted the kristw-throttle branch September 27, 2018 08:30
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
…he#5946)

* remove lodash.throttle from dependency

* add babel-plugin-lodash'

* use lodash instead of underscore for isFunction

* switch underscore to lodash

* switch from underscore to lodash flatten

* Remove slugify and use kebabCase from lodash instead
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants