-
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
Discussion. Example: registry to NP #37310
Conversation
@@ -74,7 +74,7 @@ export async function __newPlatformStart__( | |||
} | |||
|
|||
export function getNewPlatform() { | |||
if (runtimeContext.setup.core === null || runtimeContext.start.core === null) { | |||
if (runtimeContext.setup.core === null /* || runtimeContext.start.core === null */) { |
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.
My understanding was that platform loading follows
NP setup -> NP start -> legacy platform
but for some reason when legacy runs runtimeContext.start.core
is still null.
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.
Does this need to be resolved before merging?
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 don't need to merge this PR, I should have made it as a Draft. I guess, this is just an example of moving one Interpreter registry, but I am looking into moving way bigger chunk of Interpreter, as it does not have many dependencies.
But in general this runtimeContext.start.core
must be addressed somehow. For moving registries we don't need runtimeContext.start.core
, but kibana-react
will probably need runtimeContext.start.core
in legacy world.
💔 Build Failed |
Does anybody know why I'm a reviewer on this?? |
@m-adams sorry I was selecting the other Matt and apparently clicked on you, too. |
I love this idea and think it'll give us a concrete way to look at progress as we get further down the migration path, while simultaneously starting to test NP services. This basically answers the question of how we will deal with the last step in the migration process. As long as we keep re-exporting the new platform contracts from the equivalent legacy plugins, then this can all happen behind the scenes without downstream plugins being aware of what's going on. Basically replicating the pattern we've been using in Phase I as we move legacy code into legacy plugins. Then at the end of migration, we will end up with a bunch of "empty" legacy plugins that are simply re-exporting the new platform plugin contracts, at which point we can kill the legacy plugin and fully cut over to new platform. With the introduction of this step, that means the new platform migration process for each plugin would look something like this: Phase I
Phase II
Phase III (pattern established by this PR)
[Edit: Just because this step is Phase III doesn't mean it shouldn't start happening now, as long as a plugin has made it through the first two steps] |
Pinging @elastic/kibana-app-arch |
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 know this is a draft, but there are some things to know about the conventions we're adopting.
|
||
import { FunctionsRegistry } from './registries'; | ||
|
||
export const plugin = () => ({ |
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.
- All plugins at Elastic should follow the class convention of defining plugins, it's also useful to implement the
Plugin
interface for that class. - This plugin needs to export types for it's setup and start contracts.
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.
Good to know!
And then things can be imported statically from the top level of the plugin.
*/ | ||
|
||
import { Registry } from '@kbn/interpreter/common'; | ||
const { Fn } = require('@kbn/interpreter/common'); // eslint-disable-line |
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 does this line use require
instead of an import statement?
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.
Because Fn
does not appear in TypeScript .d.ts
, I believe, in @kbn/interpreter/common
, so it complains.
Half of Interpreter has been moved to the NP, next half is to be followed shortly. |
Proposal: start using NP as soon as possible, move every single line we can to NP.
This example moves interpreter function registry into NP plugin. NP allows for a plugin with the same name, in this case
interpreter
, to exist as a legacy plugin as well as an NP plugin.Following this pattern we can move everything line-by-line to the NP. This example moves just one of many registries.
We could proceed by:
Benefits of moving every single line of code we can to NP as soon as possible:
src/plugins
vssrc/legacy
folders.Our migration metric could be LOC in
src/plugins
folder vs LOC insrc/legacy
folder.Currently we have 145 LOC in plugins and 104,589 LOC in legacy. So our migration progress is 0.14%.