PR comments

docs/development/core/public/kibana-plugin-public.appmountparameters.history.md

@@ -47,7 +47,7 @@ export renderApp = ({ element, history }: AppMountParameters) => {
     // pass `appBasePath` to `basename`
     <Router history={history}>
       <Route path="/" exact component={HomePage} />
-    </BrowserRouter>,
+    </Router>,
     element
   );
 

docs/development/core/public/kibana-plugin-public.scopedhistory.block.md

@@ -14,5 +14,5 @@ block: (prompt?: string | boolean | History.TransitionPromptHook<HistoryLocation
 
 ## Remarks
 
-We prefer that applications use the `onAppLeave` API because it supports a more graceful experience that prefers a modal when possible, falling back to a confim dialog box in the beforeunload case.
+We prefer that applications use the `onAppLeave` API because it supports a more graceful experience that prefers a modal when possible, falling back to a confirm dialog box in the beforeunload case.
 

src/core/public/application/application_service.test.ts

@@ -588,6 +588,16 @@ describe('#start()', () => {
       expect(getUrlForApp('app1', { path: 'deep/link///' })).toBe('/base-path/app/app1/deep/link');
     });
 
+    it('does not append trailing slash if hash is provided in path parameter', async () => {
+      service.setup(setupDeps);
+      const { getUrlForApp } = await service.start(startDeps);
+
+      expect(getUrlForApp('app1', { path: '#basic-hash' })).toBe('/base-path/app/app1#basic-hash');
+      expect(getUrlForApp('app1', { path: '#/hash/router/path' })).toBe(
+        '/base-path/app/app1#/hash/router/path'
+      );
+    });
+
     it('creates absolute URLs when `absolute` parameter is true', async () => {
       service.setup(setupDeps);
       const { getUrlForApp } = await service.start(startDeps);
@@ -646,6 +656,26 @@ describe('#start()', () => {
       );
     });
 
+    it('appends a path if specified with hash', async () => {
+      const { register } = service.setup(setupDeps);
+
+      register(Symbol(), createApp({ id: 'app2', appRoute: '/custom/path' }));
+
+      const { navigateToApp } = await service.start(startDeps);
+
+      await navigateToApp('myTestApp', { path: '#basic-hash' });
+      expect(MockHistory.push).toHaveBeenCalledWith('/app/myTestApp#basic-hash', undefined);
+
+      await navigateToApp('myTestApp', { path: '#/hash/router/path' });
+      expect(MockHistory.push).toHaveBeenCalledWith('/app/myTestApp#/hash/router/path', undefined);
+
+      await navigateToApp('app2', { path: '#basic-hash' });
+      expect(MockHistory.push).toHaveBeenCalledWith('/custom/path#basic-hash', undefined);
+
+      await navigateToApp('app2', { path: '#/hash/router/path' });
+      expect(MockHistory.push).toHaveBeenCalledWith('/custom/path#/hash/router/path', undefined);
+    });
+
     it('includes state if specified', async () => {
       const { register } = service.setup(setupDeps);
 

src/core/public/application/scoped_history.test.ts

@@ -69,14 +69,14 @@ describe('ScopedHistory', () => {
       const gh = createMemoryHistory();
       gh.push('/app/wow');
       const replaceSpy = jest.spyOn(gh, 'replace');
-      const h = new ScopedHistory(gh, '/app/wow');
-      h.push('/first-page');
-      h.push('/second-page');
-      h.goBack();
-      h.replace('/first-page-replacement', { some: 'state' });
+      const h = new ScopedHistory(gh, '/app/wow'); // ['']
+      h.push('/first-page'); // ['', '/first-page']
+      h.push('/second-page'); // ['', '/first-page', '/second-page']
+      h.goBack(); // ['', '/first-page', '/second-page']
+      h.replace('/first-page-replacement', { some: 'state' }); // ['', '/first-page-replacement', '/second-page']
       expect(replaceSpy).toHaveBeenCalledWith('/app/wow/first-page-replacement', { some: 'state' });
-      expect(gh.length).toBe(4); // ['', '/app/wow', '/first-page-replacement', '/second-page]
       expect(h.length).toBe(3);
+      expect(gh.length).toBe(4); // ['', '/app/wow', '/app/wow/first-page-replacement', '/app/wow/second-page']
     });
 
     it('hides previous stack', () => {
@@ -87,9 +87,6 @@ describe('ScopedHistory', () => {
       const h = new ScopedHistory(gh, '/app/wow');
       expect(h.length).toBe(1);
       expect(h.location.pathname).toEqual('');
-      h.push('/new-page');
-      expect(gh.location.pathname).toEqual('/app/wow/new-page');
-      expect(h.location.pathname).toEqual('/new-page');
     });
 
     it('cannot go back further than local stack', () => {
@@ -278,10 +275,8 @@ describe('ScopedHistory', () => {
     });
   });
 
-  // TODO: this is the current behavior, but what should the behavior here be?
   describe('action', () => {
-    // TODO: terrible test name until we know what it should actually do. test here for demo purposes
-    it('works', () => {
+    it('provides last history action', () => {
       const gh = createMemoryHistory();
       gh.push('/app/wow');
       gh.push('/alpha');

src/core/public/application/scoped_history.ts

@@ -191,7 +191,7 @@ export class ScopedHistory<HistoryLocationState = unknown>
    *
    * @remarks
    * We prefer that applications use the `onAppLeave` API because it supports a more graceful experience that prefers
-   * a modal when possible, falling back to a confim dialog box in the beforeunload case.
+   * a modal when possible, falling back to a confirm dialog box in the beforeunload case.
    */
   public block = (
     prompt?: boolean | string | TransitionPromptHook<HistoryLocationState>

src/core/public/application/types.ts

@@ -350,7 +350,7 @@ export interface AppMountParameters<HistoryLocationState = unknown> {
    *     // pass `appBasePath` to `basename`
    *     <Router history={history}>
    *       <Route path="/" exact component={HomePage} />
-   *     </BrowserRouter>,
+   *     </Router>,
    *     element
    *   );
    *