add replace option to navigateToApp API
#68700
Conversation
pgayvallet
added
release_note:skip
| it('use `history.replace` instead of `history.push`', async () => { | ||
| service.setup(setupDeps); | ||
|
|
||
| const { navigateToApp } = await service.start(startDeps); | ||
|
|
||
| await navigateToApp('myTestApp', { replace: true }); |
There was a problem hiding this comment.
Only reproduced/added a subset of the navigateToApp test suite, as the suite is big and as it's internally using the same logic.
| export interface NavigateToAppOptions { | ||
| /** | ||
| * optional path inside application to deep link to. | ||
| * If undefined, will use {@link AppBase.defaultPath | the app's default path}` as default. |
There was a problem hiding this comment.
Extracted the options type, as the inline definition was starting to be too big imho.
| /** | ||
| * if true, will not create a new history entry when navigating (using `replace` instead of `push`) | ||
| * | ||
| * @remarks | ||
| * This option not be used when navigating from and/or to legacy applications. | ||
| */ | ||
| replace?: boolean; |
There was a problem hiding this comment.
@flash1293 as discussed on slack, I only implemented this option for KP apps. (KP to KP navigation). Can you confirm that it's alright?
| navigateToApp(appId: string, options?: { path?: string; state?: any }): Promise<void>; | ||
| navigateToApp(appId: string, options?: NavigateToAppOptions): Promise<void>; |
There was a problem hiding this comment.
Should I also add the option to navigateToUrl ?
There was a problem hiding this comment.
We might need to do it for the sake of consistency. However, we can wait until someone requests this functionality.
|
Pinging @elastic/kibana-platform (Team:Platform) |
src/core/public/application/types.ts
Outdated
| /** | ||
| * optional state to forward to the application | ||
| */ | ||
| state?: any; |
There was a problem hiding this comment.
nit: shouldn't it be unknown?
There was a problem hiding this comment.
unknown probably makes more sense. Can change that in this PR, that should be quite small.
| navigateToApp(appId: string, options?: { path?: string; state?: any }): Promise<void>; | ||
| navigateToApp(appId: string, options?: NavigateToAppOptions): Promise<void>; |
There was a problem hiding this comment.
We might need to do it for the sake of consistency. However, we can wait until someone requests this functionality.
| @@ -924,6 +924,63 @@ describe('#start()', () => { | |||
| await navigateToApp('baseApp:legacyApp1'); | |||
| expect(setupDeps.redirectTo).toHaveBeenCalledWith('/test/app/baseApp'); | |||
| }); | |||
|
|
|||
| describe('when `replace` option is true', () => { | |||
There was a problem hiding this comment.
optional: we could add a test with replace: false
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* add `replace` option to `navigateToApp` * use `unknown` type for state * add test when `replace` is false
Summary
Fix #68527
Add a
replaceoption to thenavigateToAppapplication API to usehistory.replaceinstead ofhistory.pushChecklist