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.

Sign up for GitHub

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

Jump to bottom

[SIEM] Start of deprecated lifecycle refactor #46293

Merged
merged 12 commits into from
Oct 2, 2019
Merged

[SIEM] Start of deprecated lifecycle refactor #46293

merged 12 commits into from
Oct 2, 2019

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Sep 20, 2019 •
edited
Loading

Summary

Resolves https://github.com/elastic/siem-team/issues/456
This PR refactors many many components from PureComponent to memo. We have been having unintentional side effects in some of our render phase lifecycle methods. This PR removes a few effects that we caught including:

  • componentWillMount in navigation/index.tsx
  • componentWillReceiveProps in navigation/index.tsx
  • componentWillReceiveProps in tab_navigation/index.tsx

It also begins a refactor to remove any lifecycle methods and replace with hooks. I had to cut myself off, and will continue in a new PR as some of the components I have my eye to refactor are a bit complicated and this PR is already quite large. The PR also removes LoadMoreTable as it is no longer in use.

Please thoroughly test, manually and with Cypress. This doesn't change functionality, just updated syntax and reduced renders. Refactored components include:

  • AddToKqlComponent
  • AutoSizer
  • DataDrivenColumns
  • DeleteTimelineModalButton
  • DragDropContextWrapperComponent
  • DraggableWrapperComponent
  • FieldsBrowser
  • FlyoutPaneComponent
  • Footer (timeline)
  • Header (timeline column)
  • HelpMenuComponent
  • HostsFilterComponent
  • HostsTableComponent
  • KueryAutocompletion
  • LastUpdatedAt
  • LazyAccordion
  • NetworkFilterComponent
  • NetworkTopNFlowTableComponent
  • NoteCards
  • Notes
  • OpenTimelineModalButton
  • PagingControl
  • Properties
  • ProviderItemBadge
  • Resizeable
  • SiemNavigationComponent
  • StatefulBodyComponent
  • StatefulEditDataProvider
  • StatefulEvent
  • StatefulEventDetails
  • StatefulFieldsBrowserComponent
  • StatefulSearchOrFilterComponent
  • StatefulTimelineComponent
  • SuperDatePickerComponent
  • TabNavigation
  • WithHoverActions

Remaining refactors will take place in the following issues:

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem

@elasticcla
Copy link

Hi @stephmilovic, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stephmilovic
Copy link
Contributor Author

jenkins, test it

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

XavierM
XavierM reviewed Sep 24, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/components/events_viewer/events_viewer.test.tsx
@@ -20,8 +20,17 @@ jest.mock('../../lib/settings/use_kibana_ui_setting');

const from = 1566943856794;
const to = 1566857456791;

// Suppress warnings about "act" until async/await syntax is supported: https://github.com/facebook/react/issues/14769
Copy link
Contributor

Choose a reason for hiding this comment

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

we might need to do something global instead of doing it for every single test files

Copy link
Contributor Author

@stephmilovic stephmilovic Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, I created a new issue for this #46469

@stephmilovic stephmilovic mentioned this pull request Sep 24, 2019
Merged
4 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

XavierM
XavierM reviewed Sep 24, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/components/help_menu/help_menu.tsx Outdated
}
}
export const HelpMenuComponent = React.memo(() => {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - you do not even need the return

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

XavierM
XavierM previously approved these changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@XavierM XavierM left a comment •
edited by stephmilovic
Loading

Choose a reason for hiding this comment

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

LGTM, I tested locally and I reviewed the code. With this PR we will be in better shape when we moved to a more hooks way app. @stephmilovic I Appreciate all the work and fix that you/we encounter during the review process.

  • stephs edit, more bugs :(

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@XavierM XavierM dismissed their stale review September 27, 2019 19:58

We found more bugs, @steph and I agree to more testing to make sure that we won't break anything with this PR

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stephmilovic
Copy link
Contributor Author

retest

@stephmilovic
Copy link
Contributor Author

After much pain, AutoSizer reverted to a class. Created a new issue: #47120

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

XavierM
XavierM approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM
I re-tested locally, I did not see any errors

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stephmilovic stephmilovic merged commit f8810d1 into elastic:master Oct 2, 2019
@stephmilovic stephmilovic deleted the deprecated-lifecycle-refactor branch October 2, 2019 17:47
@stephmilovic stephmilovic mentioned this pull request Oct 2, 2019
Merged
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request Oct 2, 2019
@stephmilovic
[SIEM] Start of deprecated lifecycle refactor (elastic#46293)
8b40a62
stephmilovic added a commit that referenced this pull request Oct 3, 2019
@stephmilovic
[SIEM] Start of deprecated lifecycle refactor (#46293) (#47151)
4d08050
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XavierM XavierM XavierM approved these changes

@FrankHassanabad FrankHassanabad Awaiting requested review from FrankHassanabad

Assignees
No one assigned
Labels
release_note:fix Team:SIEM v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stephmilovic @elasticmachine @elasticcla @FrankHassanabad @XavierM