Skip to content

[ML] Removing full lodash library imports #74742

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 12, 2020
Merged

[ML] Removing full lodash library imports #74742

merged 7 commits into from
Aug 12, 2020

Conversation

@jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Aug 11, 2020 •
edited
Loading

Removes all import _ from 'lodash' and replaces them with imports for each lodash function used.
All uses of _.has, _.first have been removed.
Most uses of _.last have been removed
All uses of _.get, _.each, _.filter _.reduce and _.find should also be removed, but can be done in a follow up PR.

@jgowdyelastic jgowdyelastic self-assigned this Aug 11, 2020
@jgowdyelastic jgowdyelastic marked this pull request as ready for review August 11, 2020 11:11
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner August 11, 2020 11:11
@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 11, 2020

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic jgowdyelastic mentioned this pull request Aug 11, 2020
Closed
alvarezmelissa87
alvarezmelissa87 reviewed Aug 11, 2020
View reviewed changes
x-pack/plugins/ml/public/application/services/results_service/result_service_rx.ts Outdated
import _ from 'lodash';
import each from 'lodash/each';
import get from 'lodash/get';
// import map from 'lodash/map';
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 Aug 11, 2020

Choose a reason for hiding this comment

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

Is this comment needed?

Copy link
Member Author

@jgowdyelastic jgowdyelastic Aug 11, 2020

Choose a reason for hiding this comment

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

nope!

alvarezmelissa87
alvarezmelissa87 approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Tested and LGTM ⚡

@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 11, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ml 1353 +236 1117

async chunks size

id value diff baseline
ml 8.0MB +116.3KB 7.9MB

page load bundle size

id value diff baseline
ml 566.9KB +66.5KB 500.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

darnautov
darnautov reviewed Aug 12, 2020
View reviewed changes
.../plugins/ml/public/application/components/annotations/annotations_table/annotations_table.js
// if the annotation is at the series level
// then pass the partitioning field(s) and detector index to the Single Metric Viewer
if (_.has(annotation, 'detector_index')) {
if (annotation.detector_index !== undefined) {
Copy link
Contributor

@darnautov darnautov Aug 12, 2020

Choose a reason for hiding this comment

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

Not sure if it's important here, but has is not the same as check for undefined. Would be safer to replace with hasOwnProperty

Copy link
Member Author

@jgowdyelastic jgowdyelastic Aug 12, 2020

Choose a reason for hiding this comment

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

I agree that has is not the same as checking for undefined, however the use of has in our code was not as a replacement for hasOwnProperty, it was only ever used to check whether a property had been set, and was not considering whether the property existed on the prototype chain.
In all other similar checks in our code base we use an undefined check, so for consistency i'd like to keep these changes.
but this is a good thing to point out!

darnautov
darnautov approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@jgowdyelastic jgowdyelastic merged commit a81059b into elastic:master Aug 12, 2020
@jgowdyelastic jgowdyelastic deleted the removing-full-lodash-library-imports branch August 12, 2020 08:24
@jgowdyelastic jgowdyelastic mentioned this pull request Aug 12, 2020
Merged
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Aug 12, 2020
@jgowdyelastic
[ML] Removing full lodash library imports (elastic#74742)
06de36d 
* [ML] Removing full lodash library imports

* more has

* fixing missing filter

* removing _

* removing unused file

* removing first use

* removing comment
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 12, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ingest-pipeli…
bbbe9d2 
…nes/processor-forms-a-d

* 'master' of github.com:elastic/kibana: (25 commits)
  [ML] Removing full lodash library imports (elastic#74742)
  [Search] Server strategy example (elastic#71679)
  [Reporting] Fix and test for Listing of Reports (elastic#74453)
  [maps] fix drawing shapes (elastic#74689)
  [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601)
  Deprecate schema-less specs in Vega (elastic#73805)
  [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287)
  Timelion deprecation doc (elastic#74508)
  [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736)
  [Lens] Add styling options for x and y axes on the settings popover (elastic#71829)
  [Maps] add initial location option that fits to data bounds (elastic#74583)
  theme function (elastic#73451)
  [data.ui.query] Write filters to query log from default editor. (elastic#74474)
  [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607)
  [Canvas][tech-debt] Convert renderers (elastic#74134)
  [security solutions][lists] Adds end to end tests (elastic#74473)
  pluralized for occurrences vs occurrence (elastic#74564)
  Update links that pointed to CONTRIBUTING.md (elastic#74757)
  [Ingest pipelines] Implement tabs in processor flyout (elastic#74469)
  [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/text_editor.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/bytes.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/circle.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/field_name_field.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/ignore_missing_field.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/convert.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/csv.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date_index_name.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dissect.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/drop.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/index.ts
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/shared.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 12, 2020
@gmmorris
Merge branch 'master' into actions/webhook-remove-header
d500411 
* master:
  [Vega] add functional tests for Vega visualization (elastic#74097)
  [ML] Removing full lodash library imports (elastic#74742)
  [Search] Server strategy example (elastic#71679)
  [Reporting] Fix and test for Listing of Reports (elastic#74453)
  [maps] fix drawing shapes (elastic#74689)
  [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601)
  Deprecate schema-less specs in Vega (elastic#73805)
  [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287)
  Timelion deprecation doc (elastic#74508)
jgowdyelastic added a commit that referenced this pull request Aug 12, 2020
@jgowdyelastic
[ML] Removing full lodash library imports (#74742) (#74832)
454b353 
* [ML] Removing full lodash library imports

* more has

* fixing missing filter

* removing _

* removing unused file

* removing first use

* removing comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darnautov darnautov darnautov approved these changes

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@walterra walterra Awaiting requested review from walterra

@qn895 qn895 Awaiting requested review from qn895

Assignees

@jgowdyelastic jgowdyelastic

Labels

chore :ml refactoring release_note:skip Skip the PR/issue when compiling release notes review v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jgowdyelastic @elasticmachine @kibanamachine @darnautov @alvarezmelissa87