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

Downgraded lodash 3.10.1 (to align with Kibana) #359

Merged
merged 1 commit into from
Feb 1, 2018
Merged

Downgraded lodash 3.10.1 (to align with Kibana) #359

merged 1 commit into from
Feb 1, 2018

Conversation

uboness
Copy link
Contributor

@uboness uboness commented Jan 31, 2018

  • created a predicate module that exports all predicate function (e.g. isString, isFunction,...)
  • created isNil predicate
  • sortable_properties no longer uses lodash sortBy and uses our comparators instaed

this change should enable using tables in Kibana.

@uboness uboness requested review from cjcenizal and nreese January 31, 2018 23:07
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks for migrating to Kibana's version of lodash

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Looks good, but had a couple questions!

@@ -1,5 +1,5 @@
import React from 'react';
import { isString } from 'lodash';
import { isString } from '../../services/predicate';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we export the predicates from the root modules, services?

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 can... thought about it... decided not to... but I can still do that.

Why decided not to? With generic services it somewhat feels off to export ALL possible utility functions under one module. the services sub-modules really don't have anything in common aside from the fact that they're all a bunch of utilities. To me it feels more appropriate to refer to the specific module when importing utilities.

But... that's my opinion... if you insist I can still export all to the higher services module

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right. In fact we should probably apply this pattern to the rest of the submodules under services.

isDate,
isNumber,
isNaN,
} from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not remove lodash entirely? Can we internalize these functions?

Copy link
Contributor Author

@uboness uboness Feb 1, 2018

Choose a reason for hiding this comment

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

we should... but it'll have to wait to a different pr... (there's more to this when it comes to removing lodash... we'll need to remove other usages as well - e.g. _.get)

- created a `predicate` module that exports all predicate function (e.g. `isString`, `isFunction`,...)
- created `isNil` predicate
- `sortable_properties` no longer uses lodash `sortBy` and uses our comparators instaed

this change should enable using tables in Kibana.
@uboness uboness merged commit bdbb1c3 into elastic:master Feb 1, 2018
@cjcenizal cjcenizal mentioned this pull request Feb 2, 2018
bevacqua pushed a commit to bevacqua/eui that referenced this pull request Feb 2, 2018
- created a `predicate` module that exports all predicate function (e.g. `isString`, `isFunction`,...)
- created `isNil` predicate
- `sortable_properties` no longer uses lodash `sortBy` and uses our comparators instaed

this change should enable using tables in Kibana.
@chandlerprall chandlerprall mentioned this pull request Feb 7, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants