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

Migrate server-side rendering service to packages #141902

Merged
merged 15 commits into from
Oct 3, 2022

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Sep 27, 2022

Summary

Fix #136040

Create the following packages:

  • @kbn/core-plugins-base-server-internal
  • @kbn/core-rendering-server-internal
  • @kbn/core-rendering-server-mocks

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Sep 27, 2022
Comment on lines +1 to +3
# @kbn/core-plugins-base-server-internal

This package contains base internal types of the `plugins` domain used across other core domains.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As encountered in some other places, and described here we had a cyclic dependency between the plugins domain and rendering/httpResources:

plugins -> CoreSetup/Start -> rendering -> plugins.

I broke the cycle by introducing this base package.

@@ -7,6 +7,7 @@
*/

export { RenderingService } from './rendering_service';
export { Fonts } from './views';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL that the security plugin was using this internal react component (see x-pack/plugins/security/server/prompt_page.tsx).

If at some point we have more usages of our internal components related to the page templating we should think about extracting them to a public utility package, but given there was one single usage of it, I just adapted the import to point to @kbn/core-rendering-server-internal (and had to export the component from the entrypoint)

Comment on lines +49 to 50
/** @internal */
export interface IRenderOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was wrongly annotated as public, this interface is only used internally.

@@ -7,3 +7,4 @@
*/

export { Template } from './template';
export { Fonts } from './fonts';
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL I learnt about this whole subdomain!

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

LGTM!

@pgayvallet pgayvallet marked this pull request as ready for review September 30, 2022 06:03
@pgayvallet pgayvallet requested review from a team as code owners September 30, 2022 06:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

In reviewing this, I realized that our SSR pages lost their styles again. I did my due diligence and bisected the repo to figure out that this PR isn't the culprit, but rather #140323. I've reopened #124490 to address this.

CleanShot 2022-09-30 at 10 20 19@2x

tl;dr security changes LGTM

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@pgayvallet pgayvallet enabled auto-merge (squash) October 3, 2022 06:52
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-rendering-server-internal - 2 +2
@kbn/core-rendering-server-mocks - 4 +4
core 30 29 -1
total +5

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/core-rendering-server-mocks - 1 +1
Unknown metric groups

API count

id before after diff
@kbn/core-rendering-server-internal - 2 +2
@kbn/core-rendering-server-mocks - 4 +4
core 2688 2686 -2
total +4

ESLint disabled in files

id before after diff
@kbn/core-rendering-server-internal - 2 +2
core 2 0 -2
total -0

Total ESLint disabled count

id before after diff
@kbn/core-rendering-server-internal - 2 +2
core 16 14 -2
total -0

History

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

@pgayvallet pgayvallet merged commit 8a298e4 into elastic:main Oct 3, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 3, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
* create empty packages

* moves files to packages

* adapt usages

* updating READMEs and packages jsons

* [CI] Auto-commit changed files from 'node scripts/generate codeowners'

* adapt more usages

* more import fixes

* fix mock method names

* export the `Fonts` component for security...

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* fix more usages again

* includes tsx files

Co-authored-by: kibanamachine <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
* create empty packages

* moves files to packages

* adapt usages

* updating READMEs and packages jsons

* [CI] Auto-commit changed files from 'node scripts/generate codeowners'

* adapt more usages

* more import fixes

* fix mock method names

* export the `Fonts` component for security...

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* fix more usages again

* includes tsx files

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate core domain services to packages: Server-side Rendering service
6 participants