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.

Sign up for GitHub

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

Configure ScopedHistory consistently regardless of URL used to mount app #75074

Merged
merged 2 commits into from
Aug 21, 2020
Merged

Configure ScopedHistory consistently regardless of URL used to mount app #75074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bfb4b4
Configure ScopedHistory consistenty regardless of URL used to mount app
joshdover Aug 17, 2020
506a08c
Merge branch 'master' into np/app-route-fix
elasticmachine Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/core/public/application/ui/app_router.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ export const AppRouter: FunctionComponent<Props> = ({
key={mounter.appRoute}
path={mounter.appRoute}
exact={mounter.exactRoute}
render={({ match: { url } }) => (
render={({ match: { path } }) => (
<AppContainer
appPath={url}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I did not fix the legacy route below. Changing it caused a lot of test failures + no apps are currently using this and it is slated for removal in #74911

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find proper documentation for it, what is the actual difference between match.url and match.path ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the docs:

path - (string) The path pattern used to match. Useful for building nested s
url - (string) The matched portion of the URL. Useful for building nested s

From what I can gather, path will always be the path that was passed to the Route component that matched, while url will be the portion of the URL that matched. I think that can include the /, whereas our path never has a trailing slash unless the application explicitly includes on in the appRoute parameter.

appPath={path}
appStatus={appStatuses.get(appId) ?? AppStatus.inaccessible}
createScopedHistory={createScopedHistory}
{...{ appId, mounter, setAppLeaveHandler, setIsMounting }}
Expand Down
7 changes: 4 additions & 3 deletions test/plugin_functional/plugins/core_plugin_a/public/application.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { History } from 'history';
import React from 'react';
import ReactDOM from 'react-dom';
import { Router, Route, withRouter, RouteComponentProps } from 'react-router-dom';
import { Router, Route, withRouter, RouteComponentProps, Redirect } from 'react-router-dom';

import {
EuiPage,
Expand Down Expand Up @@ -95,7 +95,7 @@ const Nav = withRouter(({ history, navigateToApp }: NavProps) => (
{
id: 'home',
name: 'Home',
onClick: () => history.push(''),
onClick: () => history.push('/home'),
'data-test-subj': 'fooNavHome',
},
{
Expand All @@ -122,7 +122,8 @@ const FooApp = ({ history, context }: { history: History; context: AppMountConte
<EuiPageSideBar>
<Nav navigateToApp={context.core.application.navigateToApp} />
</EuiPageSideBar>
<Route path="/" exact component={Home} />
<Route path="/" exact render={() => <Redirect to="/home" />} />
<Route path="/home" exact component={Home} />
<Route path="/page-a" component={PageA} />
</EuiPage>
</Router>
Expand Down
18 changes: 15 additions & 3 deletions test/plugin_functional/test_suites/core_plugins/applications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
});
};

const navigateTo = async (path: string) =>
await browser.navigateTo(`${PageObjects.common.getHostPort()}${path}`);

describe('ui applications', function describeIndexTests() {
before(async () => {
await PageObjects.common.navigateToApp('foo');
Expand All @@ -63,6 +66,15 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
await testSubjects.existOrFail('fooAppHome');
});

it('redirects and renders correctly regardless of trailing slash', async () => {
await navigateTo(`/app/foo`);
await waitForUrlToBe('/app/foo/home');
await testSubjects.existOrFail('fooAppHome');
await navigateTo(`/app/foo/`);
await waitForUrlToBe('/app/foo/home');
await testSubjects.existOrFail('fooAppHome');
});
Comment on lines +69 to +76
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to find a way to test this more directly but wasn't able to find a good way of doing so without depending on the implementation details of ScopedHistory. Replicating the buggy behavior in #75051 seemed like the best option available.


it('navigates to its own pages', async () => {
// Go to page A
await testSubjects.click('fooNavPageA');
Expand All @@ -72,7 +84,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide

// Go to home page
await testSubjects.click('fooNavHome');
await waitForUrlToBe('/app/foo');
await waitForUrlToBe('/app/foo/home');
await loadingScreenNotShown();
await testSubjects.existOrFail('fooAppHome');
});
Expand All @@ -86,7 +98,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide

it('navigates to app root when navlink is clicked', async () => {
await appsMenu.clickLink('Foo');
await waitForUrlToBe('/app/foo');
await waitForUrlToBe('/app/foo/home');
// await loadingScreenNotShown();
await testSubjects.existOrFail('fooAppHome');
});
Expand All @@ -105,7 +117,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide

it('can use the back button to navigate back to previous app', async () => {
await browser.goBack();
await waitForUrlToBe('/app/foo');
await waitForUrlToBe('/app/foo/home');
await loadingScreenNotShown();
await testSubjects.existOrFail('fooAppHome');
});
Expand Down