-
Notifications
You must be signed in to change notification settings - Fork 960
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.
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
[Workspace] Add workspace id in basePath #6060
Changes from 10 commits
abfdb6b
b6a1622
64b3645
9b66a28
2e388a5
80bed72
2d710b8
2a1148e
3d975ae
50648b6
e290a13
d01be23
ca2e600
6cc5e09
7b8c21c
367268d
a5fd308
3980cd7
d544f94
d4f716c
b4f0a87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,37 +29,47 @@ | |
*/ | ||
|
||
import { modifyUrl } from '@osd/std'; | ||
import type { PrependOptions } from './types'; | ||
|
||
export class BasePath { | ||
constructor( | ||
private readonly basePath: string = '', | ||
public readonly serverBasePath: string = basePath | ||
public readonly serverBasePath: string = basePath, | ||
private readonly workspaceBasePath: string = '' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider renaming By adopting a more neutral name like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree, will change that. |
||
) {} | ||
|
||
public get = () => { | ||
return `${this.basePath}${this.workspaceBasePath}`; | ||
}; | ||
|
||
public getBasePath = () => { | ||
return this.basePath; | ||
}; | ||
|
||
public prepend = (path: string): string => { | ||
if (!this.basePath) return path; | ||
public prepend = (path: string, prependOptions?: PrependOptions): string => { | ||
const { withoutWorkspace } = prependOptions || {}; | ||
const basePath = withoutWorkspace ? this.basePath : this.get(); | ||
if (!basePath) return path; | ||
return modifyUrl(path, (parts) => { | ||
if (!parts.hostname && parts.pathname && parts.pathname.startsWith('/')) { | ||
parts.pathname = `${this.basePath}${parts.pathname}`; | ||
parts.pathname = `${basePath}${parts.pathname}`; | ||
} | ||
}); | ||
}; | ||
|
||
public remove = (path: string): string => { | ||
if (!this.basePath) { | ||
public remove = (path: string, prependOptions?: PrependOptions): string => { | ||
const { withoutWorkspace } = prependOptions || {}; | ||
const basePath = withoutWorkspace ? this.basePath : this.get(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can rename the "workspace" in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was intending to rename the withoutWorkspace to withoutClientBasePath but I was thinking in the future, the clientBasePath may consist of workspace/dataSourceId/other stuff and prependOptions will have withoutWorkspace/withoutDataSourceId/withoutClientBasePath options accordingly so I kept them. But for now, there is no such case so it has no harm to have only one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thanks for the clarification^^ |
||
if (!basePath) { | ||
return path; | ||
} | ||
|
||
if (path === this.basePath) { | ||
if (path === basePath) { | ||
return '/'; | ||
} | ||
|
||
if (path.startsWith(`${this.basePath}/`)) { | ||
return path.slice(this.basePath.length); | ||
if (path.startsWith(`${basePath}/`)) { | ||
return path.slice(basePath.length); | ||
} | ||
|
||
return path; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,8 @@ import { AnonymousPathsService } from './anonymous_paths_service'; | |
import { LoadingCountService } from './loading_count_service'; | ||
import { Fetch } from './fetch'; | ||
import { CoreService } from '../../types'; | ||
import { getWorkspaceIdFromUrl } from '../utils'; | ||
import { WORKSPACE_PATH_PREFIX } from '../../utils/constants'; | ||
|
||
interface HttpDeps { | ||
injectedMetadata: InjectedMetadataSetup; | ||
|
@@ -50,9 +52,23 @@ export class HttpService implements CoreService<HttpSetup, HttpStart> { | |
|
||
public setup({ injectedMetadata, fatalErrors }: HttpDeps): HttpSetup { | ||
const opensearchDashboardsVersion = injectedMetadata.getOpenSearchDashboardsVersion(); | ||
let workspaceBasePath = ''; | ||
const plugins = injectedMetadata.getPlugins(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Is there precedence for depending on a plugin in core? i dont see any use of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but actually what I want to do here is to make the workspace id logic disabled when workspace is not enabled. But it turns out that it will introduce another indirect dependency on the workspace plugin. I can revert to previous version, in which it will try to construct workspaceBasePath no matter workspace is enabled or not, and let server side to gives a |
||
const findWorkspaceConfig = plugins.find((plugin) => plugin.id === 'workspace'); | ||
// Only try to get workspace id from url when workspace feature is enabled | ||
if (findWorkspaceConfig) { | ||
const workspaceId = getWorkspaceIdFromUrl( | ||
window.location.href, | ||
injectedMetadata.getBasePath() | ||
); | ||
if (workspaceId) { | ||
workspaceBasePath = `${WORKSPACE_PATH_PREFIX}/${workspaceId}`; | ||
} | ||
} | ||
const basePath = new BasePath( | ||
injectedMetadata.getBasePath(), | ||
injectedMetadata.getServerBasePath() | ||
injectedMetadata.getServerBasePath(), | ||
workspaceBasePath | ||
); | ||
const fetchService = new Fetch({ basePath, opensearchDashboardsVersion }); | ||
const loadingCount = this.loadingCount.setup({ fatalErrors }); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how none of the other tests needed to change if the workspace was not provided.