Skip to content
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

[APM] Remove dependency on ui/chrome in favor of newer core APIs #41868

Merged

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Jul 24, 2019

Closes #34444

Created useCore/CoreContext to pass down a single instance of core to components which were previously using ui/chrome. Those components were updated to use core APIs instead of ui/chrome.

@ogupte ogupte added the release_note:skip Skip the PR/issue when compiling release notes label Jul 24, 2019
@ogupte ogupte requested a review from a team as a code owner July 24, 2019 08:20
@ogupte ogupte self-assigned this Jul 24, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed


const CoreContext = createContext<InternalCoreStart>({} as InternalCoreStart);
const CoreProvider: React.SFC<{ core: InternalCoreStart }> = props => {
const { core, ...restProps } = props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should omit ...restProps until we need.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's used for children? Maybe make that explicit?

@@ -6,9 +6,9 @@

import { npStart } from 'ui/new_platform';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still allowed in NP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the hack pattern will go away eventually, but the npStart import is valid until we get to that point. This is how legacy plugins import the new platform core at the top-level.

import { CoreProvider } from './context/CoreContext';

const { core } = npStart;
// console.log(npStart);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ogupte ogupte force-pushed the apm-34444-new-platform-migration-ui-chrome-di branch from 6f4bf36 to dccb5cf Compare July 24, 2019 18:14
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ogupte
Copy link
Contributor Author

ogupte commented Jul 24, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ogupte ogupte merged commit b74f931 into elastic:master Jul 25, 2019
ogupte added a commit to ogupte/kibana that referenced this pull request Jul 25, 2019
…stic#41868)

* [APM] Remove dependency on ui/chrome in favor of newer core APIs

* [APM] fix broken tests by mocking useCore hook
ogupte added a commit that referenced this pull request Jul 25, 2019
) (#41947)

* [APM] Remove dependency on ui/chrome in favor of newer core APIs

* [APM] fix broken tests by mocking useCore hook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] New platform migration: Lift up ui/chrome dependency
3 participants