-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[FieldFormats] fix register
on start contract
#106828
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
@elasticmachine merge upstream |
.then(([coreStart, { indexPatternFieldEditor, uiActions, data }]) => { | ||
const suffixFormatter = getSuffixFormatter(data.fieldFormats.deserialize); | ||
if (!dataSetup.fieldFormats.has(suffixFormatter.id)) { | ||
dataSetup.fieldFormats.register([suffixFormatter]); |
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.
Why you are registering this to the upper data
plugin rather than the inner one? Can you explain with a comment?
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.
In this pr, I fixed the type definition of field formats registry so that you don't have register()
function on the start contract anymore. It was available prior to this pr because of an issue in type definitions. The only consumer who used register
on a start contract was the lens.
I don't think this needs a code comment because there is no other option to call register()
anymore.
Longer-term we plan to make all registries allow new items to be registered only in a setup contract and only during the setup lifecycle. It will likely be forbidden to call methods on the setup contract after the setup cycle has finished (just like the lens does now). This current long-long term plan. #106838 #106510 (comment)
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.
Tested on Safari 👍
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Anton Dosov <[email protected]>
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/security/users·js.security app users should show the default rolesStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/security/users·js.security app users should show the default rolesStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard_mode/dashboard_view_mode·js.dashboard mode Dashboard View Mode Dashboard viewer "before all" hook: Create dashboard only mode user for "shows only the dashboard app link"Standard Out
Stack Trace
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #106754 which apparently was a typescript mistake
Please note: Lens uses setup contracts after setup lifecycle and this shouldn't be allowed. I didn't address this #106838
Plugin API changes
Fixed a bug where field formatter start contract exposed
register
method. Now it is available only on setup contract.