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.

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

Remove browser basePath service, move functionality into browser http service #36611

Merged
merged 5 commits into from
May 22, 2019

Conversation

eliperelman
Copy link
Contributor

@eliperelman eliperelman commented May 15, 2019

Summary

The browser basePath service is a pretty small service which only has functionality for the manipulation of Kibana's basePath in order to affect HTTP requests. This patch removes the browser basePath service in favor of handling by the browser HTTP service.

Commits needing review

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Dev Docs

The browser basePath service is removed in favor of handling by the browser http service. As the basePath was used for controlling the route used to make fetch requests, it makes sense to colocate this functionality into the HTTP service.

@eliperelman eliperelman added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 v7.2.0 labels May 15, 2019
@eliperelman eliperelman requested a review from a team May 15, 2019 20:23
@eliperelman eliperelman self-assigned this May 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Some structural comments before digging into some of the details

src/core/public/http/http_service.ts Outdated Show resolved Hide resolved
src/core/public/http/types.ts Outdated Show resolved Hide resolved
@eliperelman
Copy link
Contributor Author

@joshdover made changes from your review in a312ea2.

src/core/public/http/http_service.test.ts Outdated Show resolved Hide resolved
src/test_utils/public/http_test_setup.ts Show resolved Hide resolved
src/core/public/http/http_service.ts Outdated Show resolved Hide resolved
src/core/public/http/http_service.ts Show resolved Hide resolved
src/core/public/http/types.ts Outdated Show resolved Hide resolved
@joshdover joshdover dismissed their stale review May 20, 2019 15:57

Haven't re-reviewed yet, but it looks like @restrry has spotted some things I noticed too

@eliperelman
Copy link
Contributor Author

eliperelman commented May 21, 2019

Current commits needing review have been updated in the description.

@eliperelman eliperelman force-pushed the basepath-to-http branch 3 times, most recently from d45afbd to 8f036b5 Compare May 22, 2019 14:40
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine May 22, 2019
@elastic elastic deleted a comment from elasticmachine May 22, 2019
@elastic elastic deleted a comment from elasticmachine May 22, 2019
@elastic elastic deleted a comment from elasticmachine May 22, 2019
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Looks good overall, have some thoughts on naming, but I'll leave that to you to decide.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@eliperelman eliperelman merged commit 0fb2e25 into elastic:master May 22, 2019
@eliperelman eliperelman added v7.3.0 and removed v7.2.0 labels May 22, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

eliperelman added a commit to eliperelman/kibana that referenced this pull request May 29, 2019
… service (elastic#36611)

* Remove browser basePath service, move functionality into browser http service

* Update generated documentation for removal of browser basePath

* Fix type interface for removal of basePath

* Split IHttpService into separate setup and start interfaces

* Rename appendToBasePath to prependBasePath, rename removeFromBasePath to removeBasePath
eliperelman added a commit to eliperelman/kibana that referenced this pull request May 31, 2019
… service (elastic#36611)

* Remove browser basePath service, move functionality into browser http service

* Update generated documentation for removal of browser basePath

* Fix type interface for removal of basePath

* Split IHttpService into separate setup and start interfaces

* Rename appendToBasePath to prependBasePath, rename removeFromBasePath to removeBasePath
eliperelman added a commit that referenced this pull request May 31, 2019
…r http service (#36611) (#37387)

* Remove browser basePath service, move functionality into browser http service (#36611)

* Remove browser basePath service, move functionality into browser http service

* Update generated documentation for removal of browser basePath

* Fix type interface for removal of basePath

* Split IHttpService into separate setup and start interfaces

* Rename appendToBasePath to prependBasePath, rename removeFromBasePath to removeBasePath

* Fix import of http test setup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:breaking release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants