-
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
Remove deprecated & unused public-AsyncPlugin
#127048
Remove deprecated & unused public-AsyncPlugin
#127048
Conversation
💚 Build SucceededMetrics [docs]Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
Unreferenced deprecated APIs
History
To update your PR or re-run it, just comment with: |
if (isPromise(contractOrPromise)) { | ||
if (this.coreContext.env.mode.dev) { | ||
// eslint-disable-next-line no-console | ||
console.log( | ||
`Plugin ${pluginName} is using asynchronous setup lifecycle. Asynchronous plugins support will be removed in a later version.` | ||
); | ||
} |
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.
I just wonder if we shouldn't keep this block and now throw instead of just logging a warning? It would avoid developer mistakes if a new plugin is defining async setup
or start
functions by mistake?
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.
Sorry! I had auto-merge enabled. I can create a follow-up PR to add this. Do you think it's worth it?
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.
As discussed on slack, it would be a nice to have, just to get a more explicit error during the dev process, but it's really not strictly necessary.
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.
We discussed it on Slack: if a plugin exposes a promised contract and declares it as class MyPlugin extends Plugin<Promise<Contract>>
, it's technically correct and we shouldn't throw.
The plugins using that contract will have to await the contract, which doesn't make it a good contract according to Kibana's architecture, but that's a different discussion.
If we see it's really needed in the future, we can add the validation later on.
…re-browser-errors * 'main' of github.com:elastic/kibana: (46 commits) [Reporting] Capture Kibana stopped error (elastic#127017) add updatedAt to SimpleSavedObject (elastic#126359) Remove deprecated & unused `ElasticsearchServiceStart.legacy` (elastic#127050) remove opacity for fitting line series (elastic#127176) Remove deprecated & unused `HttpServiceSetup.auth` (elastic#127056) [Lens] Show underlying data editor navigation (elastic#125983) Bump dependencies (elastic#127238) Remove deprecated & unused `public-AsyncPlugin` (elastic#127048) Remove deprecated & unused `SavedObjectsImportFailure.title` (elastic#127043) skip flaky suite (elastic#123372) [kbn/generate] add basic package generator (elastic#127095) [build] Up compression quality (elastic#127064) Made fix to broken test. Deleted all existing pipelines before test starts. FLAKY: elastic#118593 (elastic#127102) Increase timeout for Jest integration tests (elastic#127220) skip failing test suite (elastic#126949) [DOCS] Adds note for data source performance impact (elastic#127184) [Security Solution] Adds CCS privileges warning enable switch in advanced settings (elastic#124459) [App Search] Move to tabbed single tabbed JSON flyout with upload and paste options and refactor cards (elastic#127162) Update dependency chromedriver to v99 (elastic#127079) [kbn/pm] add timings for more parts of bootstrap (elastic#127157) ... # Conflicts: # x-pack/plugins/reporting/common/errors/index.ts # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
Summary
Part of #121956.
Checklist
Delete any items that are not applicable to this PR.
For maintainers