moving visualize/utils to new platform #56650
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
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
7 tasks
bcf388c to
0b4f53e
Compare
lukeelmers
reviewed
Feb 3, 2020
There was a problem hiding this comment.
All LGTM, most of my notes are just around conflicts with #56353. Depending on which merges first, we'll need to update the other PR accordingly.
| return new DefaultFieldFormat(); | ||
| const createFormat = fieldFormats.serializeFieldFormat; | ||
| const getFormat = (mapping?: SerializedFieldFormat) => { | ||
| return npStart.plugins.data.fieldFormats.deserialize(mapping as any); |
There was a problem hiding this comment.
You may be able to get rid of as any with !, or by doing if (mapping)
Suggested change
| return npStart.plugins.data.fieldFormats.deserialize(mapping as any); | |
| return mapping ? npStart.plugins.data.fieldFormats.deserialize(mapping) : undefined; |
...or:
Suggested change
| return npStart.plugins.data.fieldFormats.deserialize(mapping as any); | |
| return npStart.plugins.data.fieldFormats.deserialize(mapping!); |
| import { Vis } from '../../../../../visualizations/public'; | ||
| import { tabifyGetColumns } from '../../../../../../ui/public/agg_response/tabify/_get_columns'; | ||
|
|
||
| const getTableAggs = (vis: Vis): AggConfig[] => { |
There was a problem hiding this comment.
nit: I'm adding IAggConfig in #56353, so if that merges first, it would be preferable to use it here instead.
Comment on lines
22
to
29
| import { | ||
| convertDateRangeToString, | ||
| DateRangeKey, | ||
| } from '../../../../../legacy/ui/public/agg_types/buckets/date_range'; | ||
| import { | ||
| convertIPRangeToString, | ||
| IpRangeKey, | ||
| } from '../../../../../legacy/ui/public/agg_types/buckets/ip_range'; |
There was a problem hiding this comment.
These will also break when #56353 merges; the BWC for imports from agg_types only supports imports from the top level ui/agg_types:
import {
convertDateRangeToString,
DateRangeKey,
convertIPRangeToString,
IpRangeKey,
} from 'ui/agg_types';| import { AggConfig } from '../../../../../legacy/ui/public/agg_types'; | ||
| import { SerializedFieldFormat } from '../../../../expressions/common/types'; | ||
|
|
||
| export const serializeFieldFormat = (agg: AggConfig): SerializedFieldFormat => { |
5e35f04 to
42bde92
Compare
|
@elasticmachine merge upstream |
353584e to
7e3d5e4
Compare
|
@elasticmachine merge upstream |
347f059 to
693c2dc
Compare
flash1293
requested changes
Feb 5, 2020
| import { getTableAggs } from '../../../legacy_imports'; | ||
|
|
||
| import { Vis } from '../../../../../visualizations/public'; | ||
| import { tabifyGetColumns } from '../../../../../../ui/public/agg_response/tabify/_get_columns'; |
There was a problem hiding this comment.
There should be no legacy imports within the vis types, please re-export those via legacy_imports (same with the type import a little further up). I'm currently working on a separate PR that will break on these changes.
|
|
||
| // @ts-ignore | ||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | ||
| import { createFiltersFromEvent } from '../../../../../data/public/actions/filters/create_filters_from_event'; |
There was a problem hiding this comment.
Should we re-export this from the top level of the data plugin?
There was a problem hiding this comment.
I would consider waiting for #57177
|
Also expect breakage on #56975 🙄 |
lizozom
reviewed
Feb 10, 2020
| * under the License. | ||
| */ | ||
|
|
||
| export interface DateRangeKey { |
There was a problem hiding this comment.
Isn't this the same as the TimeRange type?
There was a problem hiding this comment.
And convertDateRangeToString looks like a function that could be exported as a utility from data/public/index, taking in a TimeRange and returning a string
lizozom
reviewed
Feb 10, 2020
| @@ -38,6 +38,7 @@ export class FieldFormatsRegistry { | |||
| protected defaultMap: Record<string, IFieldFormatConfig> = {}; | |||
| protected metaParamsOptions: Record<string, any> = {}; | |||
| protected getConfig?: GetConfigFn; | |||
| deserialize: any; | |||
There was a problem hiding this comment.
public deserialize: Function ?
lizozom
reviewed
Feb 10, 2020
| @@ -24,7 +24,13 @@ | |||
| export { HTML_CONTEXT_TYPE, TEXT_CONTEXT_TYPE } from './content_types'; | |||
| export { FieldFormat } from './field_format'; | |||
| export { FieldFormatsRegistry } from './field_formats_registry'; | |||
| export { getHighlightRequest, asPrettyString, getHighlightHtml } from './utils'; | |||
| export { | |||
| getHighlightRequest, | |||
There was a problem hiding this comment.
I removed this file and changed the way we export utils here #56975
# Conflicts: # src/legacy/core_plugins/vis_type_vislib/public/legacy_imports.ts # src/plugins/data/public/index.ts
flash1293
approved these changes
Feb 12, 2020
|
|
||
| // @ts-ignore | ||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | ||
| import { createFiltersFromEvent } from '../../../../../data/public/actions/filters/create_filters_from_event'; |
This reverts commit 3d3cbfa
|
@elasticmachine merge upstream |
lizozom
reviewed
Feb 13, 2020
|
|
||
| export class FieldFormatsRegistry { | ||
| protected fieldFormats: Map<FieldFormatId, IFieldFormatType> = new Map(); | ||
| protected defaultMap: Record<string, FieldFormatConfig> = {}; | ||
| protected metaParamsOptions: Record<string, any> = {}; | ||
| protected getConfig?: FieldFormatsGetConfigFn; | ||
| public deserialize: (mapping: SerializedFieldFormat) => IFieldFormat = () => { | ||
| return new (FieldFormat.from(identity))(); |
There was a problem hiding this comment.
The input is not being used here?
lizozom
reviewed
Feb 13, 2020
src/plugins/data/public/index.ts
Outdated
| @@ -303,7 +306,7 @@ export { | |||
| TimeHistoryContract, | |||
| } from './query'; | |||
| export * from './ui'; | |||
|
|
|||
| export { FieldFormatsStart } from './field_formats'; | |||
There was a problem hiding this comment.
No need to export this I think
Use DataPluginPublicStart['fieldFormats'] where needed
d5cca21 to
4554631
Compare
|
@elasticmachine merge upstream |
1 similar comment
|
@elasticmachine merge upstream |
# Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
55336d3 to
2c5581e
Compare
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/visualize/feature_controls/visualize_spaces·ts.Visualize visualize space with no features disabled can view existing VisualizationStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
ppisljar
added a commit
to ppisljar/kibana
that referenced
this pull request
Feb 14, 2020
# Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
ppisljar
added a commit
that referenced
this pull request
Feb 15, 2020
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 17, 2020
…re/files-and-filetree * 'master' of github.com:elastic/kibana: (139 commits) Move Ace XJSON lexer-rules, worker and utils to es_ui_shared (elastic#57563) [Upgrade Assistant] Fix filter deprecations search filter (elastic#57541) [ML] New Platform server shim: update indices routes (elastic#57685) Bump redux dependencies (elastic#53348) [Index management] Client-side NP ready (elastic#57295) change id of x-pack event_log plugin to eventLog (elastic#57612) [eventLog] get kibana.index name from config instead of hard-coding it (elastic#57607) revert allow using any path to generate fixes ui titles (elastic#57535) Fix login redirect for expired sessions (elastic#57157) Expose Vis on the contract as it requires visTypes (elastic#56968) [SIEM][Detection Engine] Fixes queries to ignore errors when signals index is not present [Remote clusters] Migrate client-side code out of legacy (elastic#57365) Fix failed test reporter for SIEM Cypress use (elastic#57240) skip flaky suite (elastic#45244) update chromedriver to 80.0.1 (elastic#57602) change slack action to only report on whitelisted host name (elastic#57582) [kbn/optimizer] throw errors into stream on invalid completion (elastic#57735) moving visualize/utils to new platform (elastic#56650) ...
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
moves utils related to fieldFormatters to data/fieldFormatters and moves getTableAggs to vislib legend (last place its used)
Dev Docs
if you are importing
FormatFactory, getFormat or createFormatfromui/visualize/utilitiesyou should upgrade to importing the new platform version of those:
getFormatusenpStart.plugins.data.fieldFormats.deserialize()createFormat(renamed toserialize) and FormatFactory type: