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

Introduced EuiInMemoryTable #390

Merged
merged 5 commits into from
Feb 21, 2018
Merged

Introduced EuiInMemoryTable #390

merged 5 commits into from
Feb 21, 2018

Conversation

uboness
Copy link
Contributor

@uboness uboness commented Feb 12, 2018

Fixes #330

A higher level component that wraps EuiBasicTable and manages its state. This is the preferred table component that should be used when all data sits in memory.

From the consumer's perspective, all the features in EuiBasicTable are supported. In addition to that, you can configure to display a built-in search bar to enable searching/filtering the table records.

Since all data is in-memory and provided to this component via the items prop, there's no need to implement any data callbacks (a la onChange) as all data filtering, pagination and sorting is handled internally.

Additional changes:

  • Changed the default pagination size options to [10, 25, 50]
  • The default page size is not picked up from first option in the page size options (thus 10 if none are configured)
  • Added error message support to EuiBasicTable
  • Added "loading" support to EuiBasicTable
  • Added "noItemsMessage" support to EuiBasicTable
  • EuiTableRowCell now supports colSpan prop
  • EuiTableRowCell now supports center alignment

@uboness
Copy link
Contributor Author

uboness commented Feb 12, 2018

@snide this one needs your love.... along with the container component, I added the following to EuiBasicTable:

  • loading indication (couldn't come up with a solid way to show loading... landed on this... obviously... feel free to work your magic here :)).
  • error message support - here as well... it's kinda basic at the moment, wanted to put the icon above the message, it failed to layout properly...
  • "no items" message - this one I think is working fine... but... again.. do your thing.

@uboness uboness removed the request for review from cjcenizal February 12, 2018 15:05
@uboness
Copy link
Contributor Author

uboness commented Feb 12, 2018

/cc @cjcenizal

@snide
Copy link
Contributor

snide commented Feb 12, 2018

Can't comment on the engineering, but functionally looks good. I like the way you're displaying hte loading bit. Only problem is pagination controls are likely to be at the bottom of the table, and your loading element is at the top. They might not see it if its a big page. We might want to use opacity or an overlay mask to hint that something is going on. I assume you don't want the users to be able to click stuff in the table while it's loading? Either way, I can come up with something.

Beyond that I'd do the following.

  • Set pagination rows to be high by default. There's really no reason for people to show only 10 rows at a time unless we're worried about performance (are we) or if it's smooshed into a dashboard. (And I guess for showing examples in docs).
  • Use the callout at the top to define the three types of tables available and why / when you would use each. Right now this new type is a little lost. I'll add a PR today that lets us #anchorlink to parts within the docs and possibly add a secondary inner page menu for navigation.

@uboness
Copy link
Contributor Author

uboness commented Feb 12, 2018

They might not see it if its a big page. We might want to use opacity or an overlay mask to hint that something is going on. I assume you don't want the users to be able to click stuff in the table while it's loading

yea... I know... I thought about masking, but I thought it would be too disruptive for the eyes (I expect loading to be very fast... so would b great to avoid "flickering" effects).

Either way, I can come up with something.

yes, please! (the main goal was to provide you with the infra to implement a proper load... )

Set pagination rows to be high by default. There's really no reason for people to show only 10 rows at a time unless we're worried about performance (are we) or if it's smooshed into a dashboard. (And I guess for showing examples in docs).

we can do that... the question is what is that default. Right now changed the default size options to 10, 25 and 50... I don't think there's a "right" default here, it really depends on personal pref (display resolution, laptop vs monitor, eyesight, etc..).... but we can just choose one and move one.

what's the policy around EUI and local storage? are we planning (or already doing so) to store personal prefs in local storage such that EUI can depend on?

Have we thought about expose EUI components dynamically rather statically? For example, instead of:

import { EuiBasicTableContainer } from 'eui';

<EuiBasicTableContainer ..../>

would could do:

// in `kibana/eui.js`
import { eui } from 'eui';
import { settings } from 'settings';

export const Eui = eui(settings);

and

import { Eui } from 'kibana/eui'

<Eui.BasicTableContainer ..../>

(basically, kibana will configure Eui with its own defaults/settings - this can be driven by both local storage, stored settings and kibana's general requirements)... Cloud can do the same thing (will probably take the settings from other places).

just an idea

@cjcenizal
Copy link
Contributor

I'm really excited about this component, @uboness! I haven't look at the code yet though -- waiting until you're ready for a review. Here are my thoughts on some of your questions:

what's the policy around EUI and local storage?

I feel like LocalStorage barely falls outside of the realm of EUI. I could see a LocalStorage abstraction living in services, but I think a good abstraction will encapsulate the way things like objects and flags are formatted when they're saved and parsed when they're read out, which will vary from application to application. So my gut tells me it's better to leave interaction with LocalStorage entirely to the consumer.

are we planning (or already doing so) to store personal prefs in local storage such that EUI can depend on?

I think this is very application and feature specific. In the case of tables, for some tables it may make sense to store the pagination value which has been selected. I don't know if it makes sense to store a set of default pagination options which will apply to all tables since as you said there's no right default. So many variables to consider, but I think the most important one is context -- how the table is being used, and what it's being used to display. In Kibana there are tables which take up entire pages, tiny vertical areas, are contained in panels, popovers, flyouts, sit on the page, etc.

I think the idea so far has been to just provide the components which props which are configurable, and put the burden on the consumer for managing preference-storage and wiring those preferences to the components via props. I think the idea of making EUI globally configurable is intriguing but not sure if there's a compelling use case for it yet.

Have we thought about expose EUI components dynamically rather statically?

I think this is a pretty neat idea from a technical perspective but in practical terms I'm not sure it will yield much benefit... I think that the appropriate default will vary so much from table to table that the table-wide defaults will probably be overridden more often than not.

@uboness
Copy link
Contributor Author

uboness commented Feb 13, 2018

@cjcenizal this is not finalized yet as @snide needs to work his magic on some of the design. That said, you can start reviewing

@snide
Copy link
Contributor

snide commented Feb 14, 2018

Can one of the JS engineers attached do a review of this? From my side this functions the way we'd expect it to, but I'm not doing most of the implementation work. @chrisronline or @jen-huang have time today?

@snide
Copy link
Contributor

snide commented Feb 15, 2018

Just an FYI. There are some minor stylistic things I'd like to clean up in this PR, but all of them should be seamless having looked through @uboness' code. I think the loading solution presented here is actually pretty solid, and I'll likely just add some some stuff to it to make it more noticable if you're at the bottom of a table. I'll also clean up the error / no results messaging, but again, nothing serious and they can all be done in a follow up PR.

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.

Great work Uri! I had a few comments. Just a reminder that we also need to update the CHANGELOG.

} from 'react';
import { formatDate } from '../../../../../src/services/format';
import { createDataStore } from '../data_store';
import { EuiBasicTableContainer } from '../../../../../src/components/basic_table';
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 import all of these from the main components module? I have a regex which will replace that with @elastic/eui in the code example.

import {
  EuiBasicTableContainer,
  EuiLink,
  EuiHealth,
  EuiButton,
  EuiFlexGroup,
  EuiFlexItem,
  EuiSwitch,
  EuiSpacer,
} from '../../../../../src/components';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea.. for whatever reason, my IDE is never totally happy when I do that... it doesn't show it as an error, but more of a warning... will change

});
};

export class Table extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Spotted a typo in the file name: "conainer_selection.js" -> "container_selection.js"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

],
text: (
<p>
The following example shows to use <EuiCode>EuiBasicTableContainer</EuiCode> along with item selection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed a word: "shows how to use"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@@ -23,6 +23,8 @@ import { ExpandedItemActions } from './expanded_item_actions';
import { EuiTableRowCell } from '../table/table_row_cell';
import { EuiTableRow } from '../table/table_row';
import { PaginationBar, PaginationType } from './pagination_bar';
import { EuiIcon } from '../icon/icon';
import { LoadingTableBody } from './loading_table_body';
Copy link
Contributor

Choose a reason for hiding this comment

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

We can fix this up later, but we should prefix LoadingTableBody, PaginationBar, and PaginationType with Eui in the event we decide to export them for consumption outside of this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea... I kinda went back and forth with that... decided for now to not prefix it as for now it's not exposed. If at some point in the future will expose it, we can rename it then.


updateLoader(loader) {
this.setState({ loader });
browserTick(() => this.table.refresh());
Copy link
Contributor

Choose a reason for hiding this comment

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

Would requestAnimationFrame substitute here?

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 guess... can you explain me why one is better than the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use it just to avoid introducing a new util which duplicates native functionality. There are performance reasons but I doubt they'll apply in this situation.

<EuiSpacer size="l"/>
<EuiBasicTableContainer
ref={(table) => this.table = table}
items={this.state.loader}
Copy link
Contributor

@cjcenizal cjcenizal Feb 15, 2018

Choose a reason for hiding this comment

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

This feels unintuitive to me -- my reaction is to ask why I'm passing in a callback to a prop called items? I think it may be simpler for consumers if we provide separate components for when the items are all in-memory and when the items are fetched asynchronously. What do you think of something like this?

{/* This one expects you to have all the available items locally available
(`isLoading` lets you control the loading state if you need it) */}

<EuiBasicTableSync
  items={this.items}
  isLoading={this.state.isLoadingItems}
  columns={columns}
  pagination={pagination}
  sorting={true}
  selection={selection}
/>

{/* This one can use the callback you provided in this example, and doesn't surface
an `isLoading` prop because it manages loading state internally */}

<EuiBasicTableAsync
  fetchItems={this.state.loader}
  columns={columns}
  pagination={pagination}
  sorting={true}
  selection={selection}
/>

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I thought it odd that we need to grab a ref to the table instance and call the refresh method on it, but I think I understand why you added it. We need a way to notify the table that the items have changed remotely and our local state needs to update to reflect that change.

If you're providing the items, as in the case of EuiBasicTableSync, this method won't be needed at all, right? Because the table won't be concerned with loading or refreshing; it just updates its appearance based on the items and isLoading props it receives.

In the other scenario where you've providing a method for fetching rows, as In the case of EuiBasicTableAsync, you'll need something like this refresh method. We could protect the encapsulation of this component somewhat by passing in a subscription method instead of getting a ref to the entire instance:

subscribeToRemoteChange = callback => {
  this.refreshTable = callback;
}

deleteItem = () => {
  /* ...deletion logic... */
  this.refreshTable();
}

render() {
  return (
    <EuiBasicTableAsync
      onFetchItems={this.state.loader}
      subscribeToRemoteChange={this.subscribeToRemoteChange}
      columns={columns}
      pagination={pagination}
      sorting={true}
      selection={selection}
    />
  );
}

You can pass the callback to this prop in the constructor of EuiBasicTableAsync:

  constructor(props) {
    super(props);

    if (this.props.subscribeToRemoteChange) {
      this.props.subscribeToRemoteChange(this.refresh);
    }
  }

Both the approach you currently have and this suggested approach result in multiple requests for a single update (one request to delete, another request to update the visible rows). It seems like if we ever optimize the API to reduce requests, we'd stop using this component and use the regular EuiBasicTable, so I do wonder if there are any actual use cases for EuiBasicTableAsync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels unintuitive to me -- my reaction is to ask why I'm passing in a callback to a prop called items? I think it may be simpler for consumers if we provide separate components for when the items are all in-memory and when the items are fetched asynchronously. What do you think of something like this?

I think we'll end up with unnecessary proliferation of tables. My immediate answer to the question "why I'm passing in a callback to a prop called items?" is "because that's an option you have - you can pass an actual list or a function that loads a list". This is one the nice things about languages like JS... you don't need to create multiple classes/components for every additional option you want to support. An alternative example to the snippet you provided with the current version is:

<EuiBasicTableContainer
  items={this.items}
  isLoading={this.state.isLoadingItems}
  columns={columns}
  pagination={pagination}
  sorting={true}
  selection={selection}
/>

<EuiBasicTableContainer
  items={this.state.loader}
  columns={columns}
  pagination={pagination}
  sorting={true}
  selection={selection}
/>

same thing - single component. If we decide it's useful we can still expose an isLoading prop on the container.. it's just that I'm not sure why you'd need that - either you have a list of items in memory that don't load remotely when you display the table and then you don't need to load anything explicitly... or you display the table when you don't necessarily have everything in memory.. then you can just pass in a callback method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're providing the items, as in the case of EuiBasicTableSync, this method won't be needed at all, right? Because the table won't be concerned with loading or refreshing; it just updates its appearance based on the items and isLoading props it receives.

if you're passing an array of items, you don't need to call this method

In the other scenario where you've providing a method for fetching rows, as In the case of EuiBasicTableAsync, you'll need something like this refresh method. We could protect the encapsulation of this component somewhat by passing in a subscription method instead of getting a ref to the entire instance:

How is the example model you're proposing is simpler? I understand your goal is the protect the abstraction, but IMO too much abstractions brings to unnecessarily complicated code. At this moment the only time someone will need to hold a ref to the table is when they want to refresh it and it's fine to just call refresh on the component itself (it's not something we invented here... you have that as well with components that you want to focus - you often hold a ref to the component and call focus() on it). This is also a scalable pattern - you can add more methods (e.g. focus) to the table and use the exact same pattern for users to access this functionality.

Both the approach you currently have and this suggested approach result in multiple requests for a single update (one request to delete, another request to update the visible rows). It seems like if we ever optimize the API to reduce requests, we'd stop using this component and use the regular EuiBasicTable, so I do wonder if there are any actual use cases for EuiBasicTableAsync.

If in some cases we'll need to optimize the API to minimize requests, we'll figure it out... and maybe yea, you can always use the EuiBasicTableContainer passing it an array. BUT, you have to be very careful before you start pre-optimizing things like that... for example, if you want to optimise the API such that delete will also return a list of items without the deleted one, that means that your delete API all of a sudden needs to know on search criteria, pagination, sorting, etc... this can become quite dirty quite fast. So I wouldn't plan for this now and only optimise things when you really need to optimise them... and optimising in one place doesn't mean optimising everywhere. So dropping the option to pass in a callback for the items - to me that would be premature optimisation and forcing all consumers to still manage too much by default.

Copy link

@epixa epixa Feb 16, 2018

Choose a reason for hiding this comment

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

Regarding the different types of sources for the item array, we can have a single component that allows people to use an existing item array or asynchronously fetch a new item array: always require a function, and always resolve the returned value of that function as a promise.

So if someone wants to fetch something from state, the'd do:

<EuiBasicTableContainer
  loadItems={this.state.loader}
/>

and if someone wants to pass in an existing array of items, they'd do:

<EuiBasicTableContainer
  loadItems={() => this.items}
/>

One component, one API with a clear name, consistent asynchronous behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And since a dataSource as a name may be somewhat overloaded with mixed feelings and despair... we can call it itemsLoader and arrayItemsLoader

Copy link

Choose a reason for hiding this comment

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

Ah, I see. On one hand, I feel we shouldn't overthink this API design and to just go with whatever is simplest for now. On the other hand, I never would have guessed based on the current API that there's this intelligence kicking in automatically based on the type of value that is being passed. Some questions immediately pop into my head as well, like what happens if I pass in pagination parameters and a static list? Perhaps I'm handling pagination myself because this particular list is tied to a broader set of data that is all being paginated together or something.

In any case, I suspect whatever solution we come up with now is going to have to be changed sometime in the future anyway as we encounter more unique situations in the wild, so I guess I still lean toward just going with what is simple for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 on keeping it simple

My stance is that in any case, users will need to read the docs to understand what props this component accepts. Even with just a prop named loadItems, users need to know the actual contract of the function they need to pass. So as long as we have everything documented, I think we should be fine.

I'm totally fine with moving to the following model:

import { EuiBasicTableContainer } from '@elastic/eui'

<EuiBasicTableContainer
   loadItems={this.loadItems}
/>

and for arrays:

import {
  EuiBasicTableContainer,
  arrayTableItemsLoader as arrayLoader
} from '@elastic/eui'

<EuiBasicTableContainer
   itemsLoader={arrayLoader(this.items)}
/>

Copy link

Choose a reason for hiding this comment

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

I don't have strong opinions about this. I think what you have now and what you propose would both have unique tradeoffs, and neither is outright bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok... we'll keep it as is for now and see if we need to adjust over time

'euiTableCellContent--truncateText': truncateText,
// We're doing this rigamarole instead of creating `euiTableCellContent--textOnly` for BWC
// purposes for the time-being.
'euiTableCellContent--overflowingContent': !textOnly,
});

return (
<td className="euiTableRowCell">
<td className="euiTableRowCell" colSpan={colSpan}>
Copy link
Contributor

@cjcenizal cjcenizal Feb 16, 2018

Choose a reason for hiding this comment

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

I think the best way to fix this is by moving {...rest} to the <td>. This would be a breaking change, but it's probably how we should have done it originally since it's much more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough... I wan't sure whether we wanted to do that or not. Another option I was considering is to expose tdAttrs property, this way the consumer knows exactly where the properties are added (the general problem with the ...rest is that it's never clear where they're added...).. eg:

<TableRowCell tdAttrs={{ colSpan }} />

But I don't have a pref here... if we decide to go for tdAttrs we need to change it in this PR... if we decide to just put all the ...rest on the <td> element, we preferably change it in a separate PR as it might have bigger impact... the good thing is that if we do it (move the ..rest to the <td> element) the user interface won't change from this implementation and we can push this as is without worrying about bwc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjcenizal what do you prefer dtAttrs or just moving the ..rest - with the former I'll make the change here, with the latter we'll need to make the change in a separate pr

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to go with the latter. 👍 to making this change in another PR.


static loadInMemoryData(items, criteria) {
if (criteria.sort) {
items = items.sort(Comparators.property(criteria.sort.field, Comparators.default(criteria.sort.direction)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I pulled down the branch and searched for .sort( and only found this example.

I guess I expected/assumed the HoC would do the actual sorting too. Is this how it works and I'm not seeing it? Or is there a good reason to avoid this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HoC?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry. Higher Order Component which is the same as High Level Component I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) got it (still need to get used to the lingo I guess)

So we need to account for two scenarios here:

  1. All data is held in memory - in which case all table functionality (pagination & sorting) is handled and computed on the client. In this case, you can pass in the full array of items (object[]) to the items prop, then the table will assume that this is the whole data and will take care of pagination and sorting for you.

  2. The data is held remotely and you don't really want to load it all in-memory - in which case the sorting and pagination functionality is pushed to the server (eg. if the data is loaded from ES, ES will take care of pagination and sorting). Here you'd pass a "loader" to the items prop that has the following signature:

(criteria) => Promise<{ item: object[], totalCount: number }>

where criteria is the criteria to pass to the server

{
  page: { index: number, size: number },
  sort?: { field: string, direction: 'asc' | 'desc' }
}

would that work for you? Per a discussion @cjcenizal @epixa and I had above, we were thinking (I'm still thinking) to move to a bit "cleaner" model where the table always accepts the loader as described in option 2 above, and then for option 1 (handling in-memory array) we can provide you an inMemoryTableItemsLoader that creates a loader out of a given array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that clarification!

I think the crux of my confusion is something I put in another comment. I'm actually not sure I understand the use case for #2.

In all my use cases (which can be limiting so take this with a necessary grain of salt), I always pass in an in memory data structure to a table. If I don't know that data structure when the component is mounted, then I just show a loading state or something while I load the data, outside the table. Once the data is ready, then render the table.

I don't quite understand why this does not work if you move to a server-side pagination model. We'd need to add at least a way to know when to refetch for newer items but maybe there is more to this than I'm realizing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure... I'm not familiar with your use cases but I can see not needing this to be server driven as a valid one. So it really depends on the use case I guess and how we want to handle things. A few examples:

  • Saved Objects (dashboards, visualizations, saved searches, etc..) - these are all object stored in ES. how many of those do you have? I don't know, it can range from just a few to literally hundreds and thousands. Do you always want to load all to memory? maybe... maybe not... I would start with not (you already have all the tools to load, paginate, sort, filter docs stored in ES... why load all in memory?

  • Watch execution history - you'd easy get to thousands here

  • reports - can get quite big as well

  • users - if all goes well and we put proper OLS in kibana, I don't see a reason for this not to get to thousands as well

  • roles - will not get to thousands for sure... a candidate to load all in memory

BUT.. there's more than just memory concerns here. There's also integrity of the data. If you load everything once into memory and display the table, you're missing on updates happening in the background - other users may add/delete dashboards/visualizations, or generate more reports, more watches will execute in the meantime. The advantage of having the table data driven by the server is that you're mostly up to date with the latest data.

const onClick = () => {
store.deleteUsers(...selection.map(user => user.id));
this.setState({ selection: [] });
this.table.refresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels weird honestly. When I think of a React component, I think of changing props and state and this is grabbing an instance reference and calling a method on it directly.

Can we just consistently pass in a defined list of items to EuiBasicTableContainer and when a delete happens, we remove the item, update the state here, and just let the React lifecycle do it's thing?

IMO, BasicTable should probably assume that items will always be provided as an array

@cjcenizal
Copy link
Contributor

@uboness I think it will help us move ahead if we can agree on the use cases we're trying to address with this component. I did a quick audit of our uses cases in Kibana and spoke with @jen-huang @bmcconaghy and @chrisronline to gather feedback. Based on our uses cases, it looks like we only have a need for a table into which you pass the items and loading/error states, but not a method for requesting the items. So basically something like this:

// Not at all married to the name... BasicTableSync, BasicTableLocal, something like that.
<EuiBasicTableSync
  items={this.items /* array */}
  loading={this.state.loading /* boolean */}
  error={this.state.error /* boolean for the default error state, node for a custom one */}
  columns={columns}
  pagination={pagination}
  sorting={true}
  selection={selection}
/>

The use cases we explored were:

  • Index management: indices are stored in a Redux store, fetched via an action, and passed to the table.
  • Index pattern fields, scripted fields, and source filters: An indexPattern model mediates between the client and the server, and provides a fields property and various getters which are used to pass items to the table.
  • Index pattern creation: An input which is separate from the table is used to fetch a lot of data from the server, which is processed and used to determine which items should be passed to the table.

In none of these use cases could we find a benefit to making the table aware of how the items are being fetched or otherwise determined.

To move forward, we were thinking that it would make sense to limit the scope of this PR to the type of table I suggested above, and then add a separate table later for use cases in which parts of the table state affect the request to the server or the table needs to know about the method with which items are fetched.

We like the idea of separate components because this simplifies the interface and implementation -- we're mostly just afraid of getting into a situation where we're trying to stuff too much functionality into a single component. We're worried we'll end up with a lot of possible permutations based on how the props interact, which will make it difficult to reason about and easy to misuse. WDYT?

@cjcenizal
Copy link
Contributor

cjcenizal commented Feb 16, 2018

Oh, and to clarify we're only talking about current use cases. I just saw your comment about pagination (EDIT: pagination, searching, sorting, etc) on the server and we all agree that this is definitely the way to go in the future, for the reasons you listed. But the current state of the code doesn't make use of these tools on the server, so we don't currently have that use case.

@uboness
Copy link
Contributor Author

uboness commented Feb 16, 2018

@cjcenizal I wholeheartedly disagree with this analysis, I just listed a bunch of places where you'd like the table to be driven remotely. Loading all data at once for all these use cases amounts to not understanding the scale of things and the nature of how users are using kibana.

@cjcenizal
Copy link
Contributor

@uboness Heh see my last comment. :)

@uboness
Copy link
Contributor Author

uboness commented Feb 16, 2018

But the current state of the code doesn't make use of pagination on the server, so we don't currently have that use case.

that's not a valid reason for me to introduce 2 different components one Async and one Sync. I would encourage moving to server driven tables asap... that doesn't mean you can use the container now for in-memory items... but we don't need to wait for a random day in the future to enable server driven approach. Also, this greaty ties to the saved object APIs in kibana... I sure hope that these APIs enable you to search for objects and not only load all at once... if the latter is the case, we have a serious issue in this API that needs to be addressed asap.

/cc @epixa and feel free to add anyone else that needs to read this

@cjcenizal
Copy link
Contributor

I just had a very good and very long Zoom with @uboness. He convinced me that it's worth trying this component out in the codebase in its current form (or close to it). If it ends up not working out then it will be another breaking change and we may have to fix some stuff in Kibana, but we can afford to experiment right now, so let's give it a shot. I'll do another round of review next week and then let's try to get this over the line.

@uboness
Copy link
Contributor Author

uboness commented Feb 17, 2018

after a long conversation with @cjcenizal, I think we can come to a middle ground. I changed the name of the component to EuiInMemoryTable (because that's what it is.. regardless of how items are loaded or provided - all the data is in-memory). And provided the requested props -message, error and loading.

updated the examples in the docs.

NOTE: this table should not serve as an excuse to always load all items in memory - loading everything in memory should be the exception rather the common practice. Based on the feedback above, it looks like we're transitioning to redux... which is great... but then try to use EuiBasicTable as much as possible (as it already provides you all the callbacks and props to drive the data from the server using redux actions) and only use EuiInMemoryTable when you absolutely need to load all data in memory (for most tables in kibana, it makes little sense to load all objects to memory... just saying)

@chrisronline @cjcenizal please have another pass on this... lets get it over with and move forward

@uboness uboness changed the title Introduced EuiBasicTableContainer Introduced EuiInMemoryTable Feb 17, 2018
@bmcconaghy
Copy link
Contributor

As a tradeoff between memory usage and performance, I would argue it often makes sense to load all the data in memory. In the case of index management, the data per index is very small (8 fields) and so it makes perfect sense to load all the index metadata all at once. The user then can page and filter at near instant speed rather than waiting on server response with each click. Of course there are other times that loading everything does not make sense, hence the suggestion for two different implementations.

@nreese
Copy link
Contributor

nreese commented Feb 20, 2018

As a tradeoff between memory usage and performance, I would argue it often makes sense to load all the data in memory

++. In my previous work environement, some of our users sat halfway across the world from the Kibana server so limiting the number of HTTP requests really helped responsiveness.

@uboness
Copy link
Contributor Author

uboness commented Feb 20, 2018

as I mentioned in one of the comments above, in some cases it may be ok... but in many cases in Kibana it doesn't make much sense.... specially those objects that are relatively dynamic - by dynamic I mean objects that update often. With some objects it's not really clear if they're dynamic or not... it changes from one use case to the other, in which case I'd always prefer to avoid premature optimisation and load the in pages. In case of index management... it also depends, but it's less critical - normally you don't manage your indices that much and when you do, normally it's done by one person. The question is what you're showing in the table - if you're showing dynamic statuses of the indices (e.g. index is recovering) you'd need to refresh you data once in a while anyway (this can also be done to the full set of data). Anyhow... that's why I changed the nature of this component - this is an In-Memory table and should serve for that. For the paginated table we have the basic table.

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.

I ❤️ how simple this is to use! Thank you for making these adjustments @uboness. I think this will help us solve a ton of our immediate problems. And I understand & agree with your caveats that we should be careful about incurring performance penalties by loading too much into memory.

@nreese @bmcconaghy @chrisronline @jen-huang Would you please take a look and provide feedback so we can merge this ASAP? I think we can merge this once we have two more approvals.

@nreese
Copy link
Contributor

nreese commented Feb 20, 2018

I am unable to start this branch. I deleted my node_modules folder and ran npm install and then npm start. npm start fails with error.

screen shot 2018-02-20 at 4 42 07 pm

@nreese
Copy link
Contributor

nreese commented Feb 21, 2018

Looks like using yarn fixed the problem.

@uboness
Copy link
Contributor Author

uboness commented Feb 21, 2018

@nreese yea... I don't use npm anymore, just yarn... do we have a requirement for everything to work in both?

@chrisronline
Copy link
Contributor

I fully understand the scaling concerns around client-side paginating, sorting and holding a ton of objects in memory. We want to build components that will scale to all use cases, but I think there is something to considering multiple options. There are a fair amount of use cases (in management at least) that can safely assume the scale of records is small and knowing that, we should be able to create an optimal experience for those use cases. Client side sorting/paginating is usually a much better UX (mainly in terms of responsiveness) and trying to introduce more of that experience when appropriate is a win across the board IMO.

With that said, it's also an interesting technical goal to create some baseline Table component that can work from two higher abstractions - InMemory and ExternallyControlled (of course, a better name). Ideally, we can reuse much of the base and just add a wrapper around that base which will handle both abstractions.

And as one last point, I'm having some crazy thoughts around possibly optimizing the experience dynamically for users. Something along the lines of:

  1. Perform initial fetch of data from server of some reasonable in-memory limit
  2. Determine if we've fetched all the data
    2a) If we have, use an InMemory solution
    2b) If we have not, use the ExternallyControlled solution

Now, this probably sounds crazy because it will introduce two different code flows from the same area but I wonder if it's something worth exploring. It's all gated on the assumption that a client-side, InMemory model is a better experience - how much better is debatable.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

} from '../search_bar';
import { EuiSpacer } from '../spacer/spacer';

const BasicTableSyncPropTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

InMemoryTablePropTypes?

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

A higher level component that wraps `EuiBasicTable` and manages its state. This is the preferred component that should be used when not using a centralized state management (e.g. redux).

From the consumer's perspective, all the features in `EuiBasicTable` are supported by this one as well, the main differences between the two are:

- no need to provide an `onChange` callback. Instead the provided `items` property can either be an array of items or a function that given a criteria returns a promise to a list the list of items -> `(criteria) => Promise<{ items: object[], totalCount: number }>`.
- pagination configuration is much simpler - you can enabled pagination by providing `pagination={true}` as prop, or if you'd like to configure the pagination size menue - `pagination={{ pageSizeOptions: [5, 10, 15] }}`.
- sorting configuration is much simpler - you can enable sorting by providing `sorting={true}`

Additional changes:

- Changed the default pagination size options to `[10, 25, 50]`
- The default page size is not picked up from first option in the page size options (thus `10` if none are configured)
- Added error message support to `EuiBasicTable`
- Added "loading" support to `EuiBasicTable`
- Added "noItemsMessage" support to `EuiBasicTable`
- `EuiTableRowCell` now supports `colSpan` prop
- `EuiTableRowCell` now supports `center` alignment
- renamed to `EuiInMemoryTable` - because that's what it is
- only accepts an array of in memory items (no loading function for now... we can add it later if we need it)
- enables you to set `message`, `error` and `loading` from the outside via props.
- since the table now encapsulates the navigation of the in-memory data, this table now enables you to configure a search bar as well
@uboness uboness merged commit 2083b37 into elastic:master Feb 21, 2018
@cjcenizal cjcenizal mentioned this pull request Feb 21, 2018
3 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.

7 participants