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

fix(channel-web): fix useSessionStorage config option #5118

Merged
merged 33 commits into from
Sep 16, 2021

Conversation

laurentlp
Copy link
Contributor

@laurentlp laurentlp commented Jun 16, 2021

This PR fixes as an issue where setting useSessionStorage to true with the webchat would not store the information in the sessionStorage as it should.

  • The storage utility now allows changing storage driver when the value of window.USE_SESSION_STORAGE changes.

  • Since the storage is initialized before the value of window.USE_SESSION_STORAGE is set, we have to re-setup the EventBus in order for it to store the socket id in the sessionStorage.

  • Adds missing typings

  • Tests were added here: chore(shared-lite): added unit tests for storage utils #5405

See botpress/studio#22 for more details

@laurentlp laurentlp marked this pull request as ready for review August 25, 2021 15:49
@laurentlp laurentlp requested review from allardy and a team August 25, 2021 15:49
Copy link
Member

@EFF EFF left a comment

Choose a reason for hiding this comment

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

I've got a few questions, let me know it it's not clear .

modules/channel-web/src/views/lite/core/socket.tsx Outdated Show resolved Hide resolved
modules/channel-web/src/views/lite/store/index.ts Outdated Show resolved Hide resolved
packages/ui-shared-lite/utils/storage.ts Outdated Show resolved Hide resolved
EFF
EFF previously approved these changes Sep 1, 2021
samuelmasse
samuelmasse previously approved these changes Sep 7, 2021
Copy link
Contributor

@samuelmasse samuelmasse left a comment

Choose a reason for hiding this comment

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

Nice work on the fix 👍 When I set useSessionStorage to true I don't get the same conversation back when closing my browser.

I think for the implementation that it would be better to only setup the socket once if useSessionStorage is passed to true in the init() function of the webchat. There seems to be a problem for example with bp/channel-web/user-lang. It gets set in the local storage at first and I need to refresh the page to have it in the session storage.

Aside from this fix I think we can merge this. I put some comments mostly to get some clarifications on the changes but they don't require changing anything

modules/channel-web/src/views/lite/core/socket.tsx Outdated Show resolved Hide resolved
packages/ui-shared-lite/yarn.lock Show resolved Hide resolved
packages/ui-shared-lite/utils/storage.ts Show resolved Hide resolved
packages/ui-shared-lite/package.json Show resolved Hide resolved
samuelmasse
samuelmasse previously approved these changes Sep 8, 2021
Copy link
Contributor

@samuelmasse samuelmasse left a comment

Choose a reason for hiding this comment

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

LGTM

* chore(sharedLite): added unit tests for storage utils

* fix build

* updated unit tests
samuelmasse
samuelmasse previously approved these changes Sep 8, 2021
Copy link
Member

@allardy allardy left a comment

Choose a reason for hiding this comment

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

As discussed, I don't think useSessionStorage should be modified after the webchat was initialized. Changing the storage means we should clear the local storage, then reload the whole page using the chosen storage. It should be chosen when the webchat is initialized only.

Currently, we load the webchat twice if useSessionStorage is enabled. Changes to the event bus & reloading of data when that property is changed just adds useless complexity

The storage lib of ui-shared is used in the studio and in some modules, while the window.USE_SESSION_STORAGE property is only handled by the webchat (in ui-lite), so changes made in the studio aren't used anywhere.

I think it could make sense to move that property in the botpress configuration file, but to keep it simple for now, just set useSessionStorage when the webchat is initialized, and log a warning if it is passed to botpressWebChat.configure

@laurentlp
Copy link
Contributor Author

As discussed, I don't think useSessionStorage should be modified after the webchat was initialized. Changing the storage means we should clear the local storage, then reload the whole page using the chosen storage. It should be chosen when the webchat is initialized only.

Changes were made and 'useSessionStorage' can only be configured once during the webchat initialization. If you try to re-configure this value calling configure, a warning is print in the console that tells you the value cannot be altered.

@laurentlp laurentlp marked this pull request as draft September 14, 2021 20:50
@laurentlp laurentlp marked this pull request as ready for review September 16, 2021 21:39
Copy link
Member

@allardy allardy left a comment

Choose a reason for hiding this comment

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

LGTM

@laurentlp laurentlp merged commit edac7c6 into master Sep 16, 2021
@laurentlp laurentlp deleted the llp_fix_session_storage branch September 16, 2021 22:29
@EFF EFF mentioned this pull request Sep 28, 2021
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.

None yet

4 participants