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

[ENDPOINT][TECH DEBT] Simplify the code to do history navigation in trusted apps (and other sections) #80143

Open
efreeti opened this issue Oct 12, 2020 · 2 comments
Assignees
Labels
Feature:SecurityAdmin Security Solution administration feature Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture

Comments

@efreeti
Copy link
Contributor

efreeti commented Oct 12, 2020

In trusted apps I've experimented with a custom hook for making navigation simpler:

https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/hooks.ts#L33

There were some comments about it being not ideal:

#79485 (comment)

The issue to be discussed solved - when implementing history navigation in trusted apps (and in other sections of endpoint management) for that matter we always have to perform couple of operations that are same all over the place:

  • retrieve current location from Redux using const location = useTrustedAppsSelector(getCurrentLocation);
  • change couple of parameters on it const newLocation = { ...location, param: 'new value' }
  • generate URI out of it const uri = getTrustedAppsListPath(newLocation);
  • call history push with that URI hostory.push(uri);

Though it's not tremendously big chunk of code it repeats often and it makes every component that wants to do navigation directly have dependencies on useHistory (react routing), useTrustedAppsSelector (useSelector, react-redux), and location type. To avoid this boilerplate I've added the mentioned above hook. As mentioned it has couple of consequences:

  • It is a new Hook/primitive which I unfortunately could not avoid as react's hooks are not a very composable primitive if you don't create custom hook (which in essence is just a function, same as useTrustedAppsSelector hook). It's debatable whether it is a big issue but it is something to write JSDoc for and developer needs to understand.
  • Due to ESLint rules for hooks I had to disable checks for some lines, because I had to remove callback parameter from the list of dependencies. This is suboptimal.
  • This hook doesn't accept list of dependencies to be used for memoisation, neither it can reliably support that.

Here are some proposed solutions:

  • Unless it's strongly insisted that we should not introduce any new primitives hooks for our convenience the new hook can be just documented and potentially moved to some common place (but then it needs to be generalised).
  • For two other problems there is a middle ground solution - the callback passed to useTrustedAppsNavigateCallback could be wrapped with useCallback on the caller side. Something like:
      useTrustedAppsNavigateCallback(
        useCallback(({ page }) => ({
          page_index: page.index,
          page_size: page.size,
        }), [])
     )

Makes it a bit bulkier but solves the memoization problems and allows to remove disabling of ESLint check.

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-management (Team:Endpoint Management)

@efreeti efreeti added the technical debt Improvement of the software architecture and operational architecture label Oct 12, 2020
@kevinlog kevinlog added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution planning labels Oct 16, 2020
@kevinlog kevinlog added v7.11.0 and removed planning labels Oct 16, 2020
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 19, 2020
@paul-tavares
Copy link
Contributor

Not sure if this helps with the issue being tracked here, but we do have this hook here with may be related:

https://github.com/elastic/kibana/blob/1216b0f7cd2c0697393547432bee343ed33b96b5/x-pack/plugins/security_solution/public/common/hooks/endpoint/use_navigate_by_router_event_handler.ts

@charlie-pichette charlie-pichette added the Feature:SecurityAdmin Security Solution administration feature label Oct 22, 2020
@kevinlog kevinlog removed the v7.11.0 label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:SecurityAdmin Security Solution administration feature Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

7 participants