Remove internal platform types exports #50427
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pgayvallet
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
pgayvallet
commented
Nov 12, 2019
src/core/server/internal_setup.ts
Outdated
Comment on lines
26
to
33
| /** @internal */ | ||
| export interface InternalCoreSetup { | ||
| context: ContextSetup; | ||
| http: InternalHttpServiceSetup; | ||
| elasticsearch: InternalElasticsearchServiceSetup; | ||
| uiSettings: InternalUiSettingsServiceSetup; | ||
| } | ||
|
|
There was a problem hiding this comment.
The server/types.ts files states This module is intended for consumption by public to avoid import issues with server-side code, so I created another file for our internal types. Tell me if this should be moved elsewhere.
There was a problem hiding this comment.
Separate file is fine to me, but I think public and server should be consistent. Also I think the name should probably be something like internal_types rather than internal_setup.
Comment on lines
811
to
815
| export interface LegacyServiceSetupDeps { | ||
| // Warning: (ae-incompatible-release-tags) The symbol "core" is marked as @public, but its signature references "InternalCoreSetup" which is marked as @internal | ||
| // Warning: (ae-forgotten-export) The symbol "InternalCoreSetup" needs to be exported by the entry point index.d.ts | ||
| // | ||
| // (undocumented) | ||
| core: InternalCoreSetup & { |
There was a problem hiding this comment.
We still needs to export LegacyServiceSetupDeps as it is used in src/legacy/server/kbn_server.d.ts, so the warning in documentation actually changes. Not sure we can do anything about this until legacy is dropped though?
There was a problem hiding this comment.
Yeah that's fine. Some of the doc warnings are ok if they're internal types.
joshdover
approved these changes
Nov 12, 2019
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dgieselaar
approved these changes
Nov 13, 2019
There was a problem hiding this comment.
LGTM. I'm migrating the routes to NP in #49455, but this seems straight forward me to deal with in that branch. This should be in quickly I assume?
|
@dgieselaar thanks for the review. And yes, this will be merged as soon as the CI finishes |
💚 Build Succeeded |
pgayvallet
added a commit
to pgayvallet/kibana
that referenced
this pull request
Nov 13, 2019
* remove exports of internal core types * updates documentation * rename internal types file
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Nov 13, 2019
* upstream/master: Remove internal platform types exports (elastic#50427) [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312) [Telemetry] Server side fetcher (elastic#50015) [SIEM] Detection engine placeholders (elastic#50220) [Uptime] Donut chart loader position centered vertically (elastic#50219) update telemetry banner notice text (elastic#50403) Fix aborting when searching without batching (elastic#49966) [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189) Revert chromedriver update (elastic#50324) Remove deprecated argument include_type_name from ES calls (elastic#50285) [Maps] add settings to maps telemetry (elastic#50161) remove visualize loader (elastic#46910) Fix misuse of react-router and react-router-dom (elastic#50120)
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Nov 13, 2019
…-fallback * 'master' of github.com:elastic/kibana: Remove internal platform types exports (elastic#50427) [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312) [Telemetry] Server side fetcher (elastic#50015) [SIEM] Detection engine placeholders (elastic#50220) [Uptime] Donut chart loader position centered vertically (elastic#50219) update telemetry banner notice text (elastic#50403) Fix aborting when searching without batching (elastic#49966) [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189) Revert chromedriver update (elastic#50324) Remove deprecated argument include_type_name from ES calls (elastic#50285) [Maps] add settings to maps telemetry (elastic#50161) remove visualize loader (elastic#46910) Fix misuse of react-router and react-router-dom (elastic#50120) Move table-list-view to kibana-react (elastic#50046) [ML] Stats bar for data frame analytics (elastic#49464)
pgayvallet
added a commit
that referenced
this pull request
Nov 13, 2019
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Nov 13, 2019
…ger-ace-theme * 'master' of github.com:elastic/kibana: (56 commits) [ML] Server info service refactor (elastic#50302) Remove internal platform types exports (elastic#50427) [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312) [Telemetry] Server side fetcher (elastic#50015) [SIEM] Detection engine placeholders (elastic#50220) [Uptime] Donut chart loader position centered vertically (elastic#50219) update telemetry banner notice text (elastic#50403) Fix aborting when searching without batching (elastic#49966) [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189) Revert chromedriver update (elastic#50324) Remove deprecated argument include_type_name from ES calls (elastic#50285) [Maps] add settings to maps telemetry (elastic#50161) remove visualize loader (elastic#46910) Fix misuse of react-router and react-router-dom (elastic#50120) Move table-list-view to kibana-react (elastic#50046) [ML] Stats bar for data frame analytics (elastic#49464) [ML] Make navigation in tests more stable (elastic#50132) Migrate authorization subsystem to the new platform. (elastic#46145) Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063) Update dependency @elastic/charts to v14 (elastic#49947) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
core.Fixes #49630
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately