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

Jump to bottom

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

merged 2 commits into from
Aug 21, 2020

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Aug 14, 2020 •
edited
Loading

Summary

Fixes #75051

This ensures that the app's base path rather than the current URL is used to configure the ScopedHistory instance passed to an application. This allows for more consistent behavior when mounting an app when the URL is /app/<appId>/ and /app/<appId>.

Release Note

A bug was fixed that caused some applications to not correctly render when a trailing slash was included at the end of their URLs.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cauemarcondes
Copy link
Contributor

@joshdover I can confirm that the change fixes the bug.
ezgif com-video-to-gif (3)

joshdover
joshdover commented Aug 17, 2020
View reviewed changes
src/core/public/application/ui/app_router.tsx
<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.

test/plugin_functional/test_suites/core_plugins/applications.ts
Comment on lines +69 to +76
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');
});
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.

@joshdover joshdover added backport:skip This commit does not require backporting Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0 labels Aug 17, 2020
@joshdover joshdover marked this pull request as ready for review August 17, 2020 17:23
@joshdover joshdover requested a review from a team as a code owner August 17, 2020 17:23
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@joshdover joshdover added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting labels Aug 17, 2020
@joshdover joshdover added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 17, 2020
@joshdover
Copy link
Contributor Author

@elasticmachine merge upstream

@joshdover joshdover merged commit 384213f into elastic:master Aug 21, 2020
@joshdover joshdover deleted the np/app-route-fix branch August 21, 2020 17:29
@joshdover joshdover changed the title Configure ScopedHistory consistenty regardless of URL used to mount app Configure ScopedHistory consistently regardless of URL used to mount app Aug 21, 2020
@joshdover joshdover mentioned this pull request Aug 21, 2020
Merged
joshdover added a commit to joshdover/kibana that referenced this pull request Aug 21, 2020
@joshdover @elasticmachine
Configure ScopedHistory consistenty regardless of URL used to mount a…
2544a80 
…pp (elastic#75074)

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 21, 2020
@gmmorris
Merge branch 'master' into actions/webhook-remove-header
452f964 
* master: (71 commits)
  [Lens] Show 'No data for this field' for empty field in accordion (elastic#73772)
  Skip failing lens test
  Configure ScopedHistory consistenty regardless of URL used to mount app (elastic#75074)
  Fix returned payload by "search" usage collector (elastic#75340)
  [Security Solution] Fix missing key error (elastic#75576)
  Upgrade EUI to v27.4.1 (elastic#75240)
  Update datasets UI copy to data streams (elastic#75618)
  [Lens] Register saved object references (elastic#74523)
  [DOCS] Update links to Beats documentation (elastic#70380)
  [Enterprise Search] Convert our `public_url` route to `config_data` and collect initialAppData (elastic#75616)
  [Usage Collection Schemas] Remove Legacy entries (elastic#75652)
  [Dashboard First] Lens Originating App Breadcrumb (elastic#75470)
  Improve login UI error message. (elastic#75642)
  [Security Solution] modify circular deps checker to output images of circular deps graphs (elastic#75579)
  [Data Telemetry] Add index pattern to identify "meow" attacks (elastic#75163)
  Migrate CSP usage collector to `kibana_usage_collection` plugin (elastic#75536)
  [Console] Get ES Config from core (elastic#75406)
  [Uptime] Add delay in telemetry test (elastic#75162)
  [Lens] Use index pattern service instead saved object client (elastic#74654)
  Embeddable input (elastic#73033)
  ...
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
core 1.2MB +3.0B 1.2MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

joshdover added a commit that referenced this pull request Aug 24, 2020
@joshdover @elasticmachine
[7.x] Configure ScopedHistory consistenty regardless of URL used to m…
c1fa192 
…ount app (#75074) (#75684)

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees
No one assigned
Labels
release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Page is not loaded when typing the apps home page url with / at the end of the URL
5 participants
@joshdover @cauemarcondes @elasticmachine @kibanamachine @pgayvallet