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

[Security Solution] kbn package for generic hook utils #101976

Merged
merged 9 commits into from
Jun 17, 2021

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Jun 10, 2021

Summary

This abstracts a package of generic hooks and utilities being consumed in a few other plugins. The possibility of such a package was identified as part of #100375, but the need for a new useObservable hook and no great place to put it were good enough reasons to create this package.

Checklist

For maintainers

rylnd added 4 commits June 10, 2021 17:30
Updates references, and fixes a few missing config that were preventing
packages from building.
Similar to useAsync (a nearly identical interface), this is meant to
wrap a thunk returning an observable, allowing conditional invocation
and progressive updates as the observable continues to emit.
@rylnd rylnd added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0 labels Jun 10, 2021
@rylnd rylnd self-assigned this Jun 10, 2021
@rylnd rylnd requested review from a team as code owners June 10, 2021 23:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

This function (and its tests) were moved to the hook-utils package; this
was simply missed.
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for breaking this package out! Makes sense

@rylnd
Copy link
Contributor Author

rylnd commented Jun 14, 2021

@elasticmachine merge upstream

kibanamachine and others added 2 commits June 14, 2021 14:37
The build system does not currently support these typescript features.
While a valid fix would also have been to build separate browser and
node targets a la elastic#99390, the use here was very minimal and so changing
to a supported syntax was the most pragmatic fix.
@rylnd
Copy link
Contributor Author

rylnd commented Jun 15, 2021

@mistic @jbudz would one of you be able to verify that this package builds correctly for all target environments? I recall @FrankHassanabad that should be done before merging this.

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

lgtm

@rylnd
Copy link
Contributor Author

rylnd commented Jun 17, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lists 219 221 +2
securitySolution 2184 2186 +2
total +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lists 270.7KB 271.7KB +1.0KB
securitySolution 6.9MB 6.9MB +1.2KB
total +2.2KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @rylnd

@rylnd rylnd merged commit ac07ebb into elastic:master Jun 17, 2021
@rylnd rylnd deleted the secsol-hook-utils branch June 17, 2021 17:09
rylnd added a commit to rylnd/kibana that referenced this pull request Jun 17, 2021
* Adds boilerplate for new hook-utils package

* Move existing, identified utils into our hook-utils package

Updates references, and fixes a few missing config that were preventing
packages from building.

* Extracts a common type and adds a little more JSdoc for clarity

* Adds new useObservable hook

Similar to useAsync (a nearly identical interface), this is meant to
wrap a thunk returning an observable, allowing conditional invocation
and progressive updates as the observable continues to emit.

* Remove orphaned test

This function (and its tests) were moved to the hook-utils package; this
was simply missed.

* Remove optional chaining from kbn package

The build system does not currently support these typescript features.
While a valid fix would also have been to build separate browser and
node targets a la elastic#99390, the use here was very minimal and so changing
to a supported syntax was the most pragmatic fix.

* Update old reference in test file

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	package.json
rylnd added a commit that referenced this pull request Jun 17, 2021
…2539)

* Adds boilerplate for new hook-utils package

* Move existing, identified utils into our hook-utils package

Updates references, and fixes a few missing config that were preventing
packages from building.

* Extracts a common type and adds a little more JSdoc for clarity

* Adds new useObservable hook

Similar to useAsync (a nearly identical interface), this is meant to
wrap a thunk returning an observable, allowing conditional invocation
and progressive updates as the observable continues to emit.

* Remove orphaned test

This function (and its tests) were moved to the hook-utils package; this
was simply missed.

* Remove optional chaining from kbn package

The build system does not currently support these typescript features.
While a valid fix would also have been to build separate browser and
node targets a la #99390, the use here was very minimal and so changing
to a supported syntax was the most pragmatic fix.

* Update old reference in test file

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	package.json
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 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants