fix ScopedHistory.createHref to prepend location with scoped history basePath

[pgayvallet]

Summary

Fix #62016

• Change ScopedHistory.createHref default behavior to add the scoped history's path path to the location
• add an option to directly call parent's history createHref without this behavior.

Checklist

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

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

[pgayvallet]

I think that in some case, plugins might want to be able to use createHref to create href pointing outside of their basePath, so I added this optional parameter.

Being optional, it preserves the History interface

[Dosant]

I'd also expect:

expect(h.createHref({})).toEqual(`/app/wow`);

but looks like returns ''.

[pgayvallet]

prependBasePath was returning undefined when location.pathname was undefined. Changed it to return basepath instead in 460abe1

[Dosant]

I started working on #58137 and used this pr.
Looks like in real world the basename would have a trailing slash. (at least this is the case for my example plugin)
so for this test it would be instead of

const h = new ScopedHistory(gh, '/app/wow');

it would be:

const h = new ScopedHistory(gh, '/app/wow/');

And then this test would fail with: /app/wow//new-page?alpha=true (2 slashes)

[pgayvallet]

You are right. This was caused by

  private prependBasePathToString(path: string): string {
    path = path.startsWith('/') ? path.slice(1) : path;
    return path.length ? `${this.basePath}/${path}` : this.basePath;
  }

Where ${this.basePath}/${path} always inserts a / between the paths.

Fixed in 460abe1

[pgayvallet]

prependBasePathToString was incorrectly removing the trailing /, as this is fixed, I adapted the test plugins instead or changing every waitForUrlToBe('/app/foo'); to waitForUrlToBe('/app/foo/'); in test/plugin_functional/test_suites/core_plugins/applications.ts

[Dosant]

Gave it another try with state syncing utils examples and works great. Didn't notice anything else 👍

[pgayvallet]

@Dosant I think I will need to revert the change from your #62407 (comment) comment.

expect(h.createHref({})).toEqual(/app/wow);

It introduced a regression in part of the code using history.push for a location without a pathname to only change the actual search part of the url.

kibana/x-pack/plugins/endpoint/public/applications/endpoint/view/hosts/index.tsx

Lines 78 to 87 in 65a111f

	<EuiLink
	data-test-subj="hostnameCellLink"
	href={'?' + urlFromQueryParams({ ...queryParams, selected_host: id }).search}
	onClick={(ev: React.MouseEvent) => {
	ev.preventDefault();
	history.push(urlFromQueryParams({ ...queryParams, selected_host: id }));
	}}
	>
	{hostname}
	</EuiLink>

history.push / history.createHref can accept a location without pathname, and that would create an url composed only of the search for example

history.create({ search: 'foo=bar'}) == '/?foo=bar'

changing that to

history.create({ search: 'foo=bar'}) == '/app/wow?foo=bar'

may not be such a good idea, or at least it's a breaking change in our ScopeHistory API.

@joshdover WDYT? Are the usage in kibana/x-pack/plugins/endpoint/public/applications/endpoint/view/hosts/index.tsx supposed correct, or should they specify a pathname?

[joshdover]

@joshdover WDYT? Are the usage in kibana/x-pack/plugins/endpoint/public/applications/endpoint/view/hosts/index.tsx supposed correct, or should they specify a pathname?

I believe this usage is valid, at least when comparing to using a raw History instance:

const history = createBrowserHistory({ basename: "/app/wow" });
history.push({ pathname: "/page2" });
history.push({ search: "foo=bar" });
console.log(history.location);
// => {search: "?foo=bar", pathname: "/page2", hash: ""}

I think we should be aiming to make the behavior of ScopedHistory match regular History as much as possible. If we look at how raw History works:

const history = createBrowserHistory({ basename: '/app/wow' });
console.log(history.createHref({ search: 'foo=bar' }))
// => /app/wow/?foo=bar 

We see that it does include the configured 'basename', even if no pathname is specified. I think it makes sense that createHref should always return full absolute paths, so even if pathname is undefined, we should still receive the entire path including Kibana's basePath and the appBasePath.

However, for history.push and history.replace it seems we should reuse the current pathname when none is specified by the caller:

  public push = (
    pathOrLocation: Path | LocationDescriptorObject<HistoryLocationState>,
    state?: HistoryLocationState
  ): void => {
    this.verifyActive();
    if (typeof pathOrLocation === 'string') {
      this.parentHistory.push(this.prependBasePath(pathOrLocation), state);
    } else {
+     pathOrLocation = {
+       pathname: this.location.pathname,
+       ...pathOrLocation
+     };
      this.parentHistory.push(this.prependBasePath(pathOrLocation));
    }
  };

  /**
   * Replaces the current location in the history stack. Does not remove forward or backward entries.
   *
   * @param pathOrLocation a string or location descriptor
   * @param state
   */
  public replace = (
    pathOrLocation: Path | LocationDescriptorObject<HistoryLocationState>,
    state?: HistoryLocationState
  ): void => {
    this.verifyActive();
    if (typeof pathOrLocation === 'string') {
      this.parentHistory.replace(this.prependBasePath(pathOrLocation), state);
    } else {
+     pathOrLocation = {
+       pathname: this.location.pathname,
+       ...pathOrLocation
+     };
      this.parentHistory.replace(this.prependBasePath(pathOrLocation));
    }

Does that adequately solve both cases?

[pgayvallet]

After a quick slack discussion with @joshdover we decided to revert the change on prependBasePath to avoid impacting push and replace, and to inject the basepath in createHref instead when pathname is not specified. This should address the issue regarding createHref without introducing breaking change on the other methods that are remaining as close as possible as History implementation

[joshdover]

nit: could we use an options object for this to make adding things in the future non-breaking?

Suggested change

	prependBasePath: boolean = true
	{ prependBasePath = true }: { prependBasePath?: boolean } = {}

[pgayvallet]

👍 Don't even know why I didn't do it that way in the first place. Will do.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 1fb7fc6

History

• 💚 Build #39511 succeeded 737c5df
• 💔 Build #39451 failed e4b8eeb
• 💔 Build #39442 failed 7604932
• 💔 Build #39279 failed ab763a3
• 💔 Build #39221 failed 3762fde

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