Skip to content

Conversation

pd-redis
Copy link
Collaborator

In this PR, I am checking if envDependent flag is off, and if it is, do not perform API request to list/get/delete Workbench commands.
Also, if the flag is off, I am storing results of sendWBCommandAction and sendWBCommandClusterAction in localStorage, from where they can be loaded after

Copy link
Contributor

@rsergeenko rsergeenko left a comment

Choose a reason for hiding this comment

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

IMO you should not use localStorage to save history of workbench, better to use IndexedDB API. LocalStorage has limited size of memory, also we limit the size of history results (only 20 results per database)

Copy link
Contributor

@rsergeenko rsergeenko left a comment

Choose a reason for hiding this comment

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

Please, also be sure that users can work with different databases and see results per database

@pd-redis
Copy link
Collaborator Author

IMO you should not use localStorage to save history of workbench, better to use IndexedDB API. LocalStorage has limited size of memory, also we limit the size of history results (only 20 results per database)

I know that localStorage is limited, but still fas few MB size (about 5). Do you think it is reasonable to expect that 20 commands can exceed that?

@rsergeenko
Copy link
Contributor

IMO you should not use localStorage to save history of workbench, better to use IndexedDB API. LocalStorage has limited size of memory, also we limit the size of history results (only 20 results per database)

I know that localStorage is limited, but still fas few MB size (about 5). Do you think it is reasonable to expect that 20 commands can exceed that?

yes, even 1 command result can exceed localStorage size. Users may have several databases, in this case they will have (number of databases * 20) results. Also 1 command can be in group mode and contain several results of commands.

@pd-redis pd-redis closed this Nov 20, 2024
@pd-redis pd-redis reopened this Nov 20, 2024
@pd-redis
Copy link
Collaborator Author

IMO you should not use localStorage to save history of workbench, better to use IndexedDB API. LocalStorage has limited size of memory, also we limit the size of history results (only 20 results per database)

I know that localStorage is limited, but still fas few MB size (about 5). Do you think it is reasonable to expect that 20 commands can exceed that?

yes, even 1 command result can exceed localStorage size. Users may have several databases, in this case they will have (number of databases * 20) results. Also 1 command can be in group mode and contain several results of commands.

OK, I will look into it

@pd-redis pd-redis requested a review from rsergeenko November 21, 2024 10:59
@pd-redis
Copy link
Collaborator Author

IMO you should not use localStorage to save history of workbench, better to use IndexedDB API. LocalStorage has limited size of memory, also we limit the size of history results (only 20 results per database)

I know that localStorage is limited, but still fas few MB size (about 5). Do you think it is reasonable to expect that 20 commands can exceed that?

yes, even 1 command result can exceed localStorage size. Users may have several databases, in this case they will have (number of databases * 20) results. Also 1 command can be in group mode and contain several results of commands.

Implemented indexedDb storing

Copy link
Contributor

@rsergeenko rsergeenko left a comment

Choose a reason for hiding this comment

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

I think its better to move class IndexedDbStorage to a different file. Also, I would recommend to have a file with instances of different tables (export const wbHistoryStorage = new IndexedDbStorage('RI_WB_HISTORY'))

IMO it's better to have a separate record for each command execution (like implemented on BE side) than modify each time one.

Will limit of records for 1 database be implemented?

@pd-redis
Copy link
Collaborator Author

pd-redis commented Nov 21, 2024

I think its better to move class IndexedDbStorage to a different file. Also, I would recommend to have a file with instances of different tables (export const wbHistoryStorage = new IndexedDbStorage('RI_WB_HISTORY'))

Sure, moving it to separate file makse sense. I don't think we'll have other databases besides wb history one, at least for now. Which is why, I didn't bring external library to manage this.

IMO it's better to have a separate record for each command execution (like implemented on BE side) than modify each time one.

You mean like, every response from POST /workbench/command-executions to be separate record?

Will limit of records for 1 database be implemented?

yeah, this is stil Work In Progress

@rsergeenko
Copy link
Contributor

rsergeenko commented Nov 21, 2024

You mean like, every response from POST /workbench/command-executions to be separate record?

yes, I think you can try to use indexes https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/index

@rsergeenko
Copy link
Contributor

@pd-redis What will be if we need to add one more table/storage?

I think it will be better to use the library (https://dexie.org/) to be more flexible and stable.
Also, I think we should discuss how to work with local database and sql (maybe we can implement one solution for all build types?)
@ArtemHoruzhenko @zalenskiSofteq - what do you think?

Copy link
Contributor

@seth-riedel seth-riedel left a comment

Choose a reason for hiding this comment

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

Please see comments. I think we also need a lot of tests for this, both for the changes to the workbench slice as well as the behavior of services/workbenchStorage.ts

@pd-redis
Copy link
Collaborator Author

@pd-redis What will be if we need to add one more table/storage?

I think it will be better to use the library (https://dexie.org/) to be more flexible and stable. Also, I think we should discuss how to work with local database and sql (maybe we can implement one solution for all build types?) @ArtemHoruzhenko @zalenskiSofteq - what do you think?

Reg dexie - we discussed it, but the consensus was, that bringing 90K dependency just to store WB history is not worth it.
Otherwise, I have used it before and I like it.

@ArtemHoruzhenko
Copy link
Collaborator

@pd-redis will this approach support storing other data in the future? Are we ready to add another collection to idb without migrating existing data?

@pd-redis pd-redis merged commit 114a78b into feature/dynamic-dependencies Dec 5, 2024
14 checks passed
@pd-redis pd-redis deleted the fe/feature/CR-215-workbench-local-storage-history branch December 5, 2024 10:52
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.

4 participants