rdkcentral / Lightning-SDK

SDK for Lightning framework
Apache License 2.0
130 stars 69 forks source link

Kill previously keepAlive page instance #390

Closed jay-patel9 closed 1 year ago

jay-patel9 commented 1 year ago

Router: Kill previously keepAlive page instance

Here is the scenario -

Page1 -> Page2(keepAlive:true) -> Page3 -> Page4 -> Page2(same instance will be used)

The fix includes replacing the used instance with the instance created at the startup of the App.

Page1 -> Page2(keepAlive:true) -> Page3 -> Page4 -> Page2(new instance after calling deletePage(Page2))

sandeep-vedam commented 1 year ago

Hello @jay-patel9

Thanks for the PR. Regarding this fix I thoroughly tested the provided fix. It functions perfectly in cases where the route path doesn't involve any route parameters. However, I encountered a minor setback when the route contains parameters. Unfortunately, the fix doesn't seem to address this specific scenario.

As an example, let's consider the following routes:

{ path: 'viewall/:category', component: ViewAll, widgets: ['AppBackground'] }, { path: 'video', component: VideoHome, widgets: ['AppBackground'], } During testing, I tried the following approach:

Router.deletePage('viewall') - Unfortunately, this method didn't work. Router.deletePage('viewall/:category') - However, this approach did work successfully.

It appears that we need to provide the exact route path as specified in the routes.js file when using the deletePage function. I'm unsure whether this behavior is intentional or if it indicates a potential issue. It might be worth investigating further to determine the expected behavior in this situation.

jay-patel9 commented 1 year ago

Hello @jay-patel9

Thanks for the PR. Regarding this fix I thoroughly tested the provided fix. It functions perfectly in cases where the route path doesn't involve any route parameters. However, I encountered a minor setback when the route contains parameters. Unfortunately, the fix doesn't seem to address this specific scenario.

As an example, let's consider the following routes:

{ path: 'viewall/:category', component: ViewAll, widgets: ['AppBackground'] }, { path: 'video', component: VideoHome, widgets: ['AppBackground'], } During testing, I tried the following approach:

Router.deletePage('viewall') - Unfortunately, this method didn't work. Router.deletePage('viewall/:category') - However, this approach did work successfully.

It appears that we need to provide the exact route path as specified in the routes.js file when using the deletePage function. I'm unsure whether this behavior is intentional or if it indicates a potential issue. It might be worth investigating further to determine the expected behavior in this situation.

Hi @sandeep-vedam

Thanks for the feedback, I had a look at the above scenario. There can be two ways of looking at this -

  1. I can match the components Map key with the route passed, this can lead to deleting of other routes if multiple match is found which is not ideal.
{
  path: 'page5/:id',
  component: Page5,
},
{
  path: 'page5',
  component: Page6,
},

Router.deletePage('page5') will delete both pages from memory.

  1. Use the full route path to delete the page. I think this is the best way to ensure the selected page gets deleted and does not have any ambiguities.

Let me what are your thoughts.

sandeep-vedam commented 1 year ago

Hey @erikhaandrikman , @michielvandergeest , @uguraslan ,

Please let us know your thoughts on this

erikhaandrikman commented 1 year ago

Hi @jay-patel9 i suggest going for option 2 and use the full route to delete the page. So i think this PR is good to go