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

[Enterprise Search] Tech debt/cleanup: remove I/E/T Typescript prefixes #83099

Merged
merged 20 commits into from
Nov 10, 2020

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Nov 10, 2020

Summary

Follow up to #83027, which was reverted due to typescript conflicts.

Summary from previous PR:

Per a recent team discussion, we've agreed to drop the blanket I/E/T prefixes we were using on our Typescript definitions for the following reasons:

  • Typescript's examples & docs don't do this
  • Kibana doesn't do this (generally)

The exception to this rule is if the exported interface is the same name as the exported class/component - for example, Router/IRouter (used by Kibana), and EnterpriseSearchRequestHandler/IEnterpriseSearchRequestHandler.

The only remaining instances of I in front of an interface name are:

  • IEnterpriseSearchRequestHandler (same name as exported class)
  • IMockRouter (same reason as IRouter)
  • ISourceRow (same name as exported SourceRow component)
  • INavContext (same name exported NavContext)
  • IFlashMessage (similar to <FlashMessages /> so kept the I to reduce confusion)

Checklist

  • Jest tests pass
  • Typescript test passes

- Types should not be exported
- Types should not be used outside each affected file
Kea now takes care of type checking for us, so there should virtually never be a need to export our values and actions interfaces going forward
Opionionated change: it was only being used for IFlashMessage, and at this point I think it's more useful to go in one level deeper to grab the type you need
- plugin.ts - remove unnecessary export
- MockRouter - remove prefix for request type, change IMockRouter to match Kibana's IRouter
- check_access - remove prefixes
- callEnterpriseSearchConfigAPI - remove prefixes
- EnterpriseSearchRequestHandler - remove prefixes
+ remove unnecessary export from public/plugin.ts
+ GroupLogic and GroupsLogic refactor - remove unnecessary defs in actions, it's already defined in the Actions interface above and in some cases (e.g. old History param) is causing out of date issues
- TSpacerSize -> SpaceSizeTypes
- SourcePriority - remove prefixes
- IComponentLoader - this isn't used anywhere else and appears to be component props so it probably should live only within component_loader.tsx
- Remove recent feed activity prefix
@cee-chen cee-chen requested review from scottybollinger and a team November 10, 2020 20:15
@cee-chen cee-chen added backport:skip This commit does not require backporting Feature:Plugins v7.11.0 labels Nov 10, 2020
@scottybollinger scottybollinger added the release_note:skip Skip the PR/issue when compiling release notes label Nov 10, 2020
@cee-chen cee-chen removed the backport:skip This commit does not require backporting label Nov 10, 2020
Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

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

🎉

@cee-chen cee-chen changed the title Remove ts prefixes [Enterprise Search] Tech debt/cleanup: remove I/E/T Typescript prefixes Nov 10, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
enterpriseSearch 678.8KB 678.8KB -42.0B

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

@cee-chen cee-chen merged commit 2d5de2b into elastic:master Nov 10, 2020
@cee-chen cee-chen deleted the remove-ts-prefixes branch November 10, 2020 22:01
cee-chen pushed a commit to cee-chen/kibana that referenced this pull request Nov 10, 2020
…es (elastic#83099)

* [All] Remove prefixes on simple self-contained type defs

- Types should not be exported
- Types should not be used outside each affected file

* [All][kea] Remove ts prefixes and unnecessary exports

Kea now takes care of type checking for us, so there should virtually never be a need to export our values and actions interfaces going forward

* [shared] Remove createHref type prefixes

* [shared] Remove breadcrumb prefixes

* [shared] Remove telemetry prefixes

* [shared] remove types.ts

Opionionated change: it was only being used for IFlashMessage, and at this point I think it's more useful to go in one level deeper to grab the type you need

* [server] remove route dependencies prefixes

* [server] Various type cleanups

- plugin.ts - remove unnecessary export
- MockRouter - remove prefix for request type, change IMockRouter to match Kibana's IRouter
- check_access - remove prefixes
- callEnterpriseSearchConfigAPI - remove prefixes
- EnterpriseSearchRequestHandler - remove prefixes

* [common] Remove InitialAppData prefix

+ remove unnecessary export from public/plugin.ts

* [common] Remove Meta prefixes

* [common] Remove configured limits prefixes

* [AS] Remove Account and Role prefixes

* [AS] Remove Engine prefixes

* [AS] Remove credentials prefixes

* [AS] Remove log settings prefixes

* [WS] Remove account/organization/initial data prefixes

* [WS] Remove group(s), user, & content source prefixes

+ GroupLogic and GroupsLogic refactor - remove unnecessary defs in actions, it's already defined in the Actions interface above and in some cases (e.g. old History param) is causing out of date issues

* [WS] Misc type fixes

- TSpacerSize -> SpaceSizeTypes
- SourcePriority - remove prefixes
- IComponentLoader - this isn't used anywhere else and appears to be component props so it probably should live only within component_loader.tsx
- Remove recent feed activity prefix

* [WS][Opinionated] Move interfaces not used in server/ out of common/ and to public/

* Fix recently rebased types
cee-chen pushed a commit that referenced this pull request Nov 11, 2020
…es (#83099) (#83129)

* [All] Remove prefixes on simple self-contained type defs

- Types should not be exported
- Types should not be used outside each affected file

* [All][kea] Remove ts prefixes and unnecessary exports

Kea now takes care of type checking for us, so there should virtually never be a need to export our values and actions interfaces going forward

* [shared] Remove createHref type prefixes

* [shared] Remove breadcrumb prefixes

* [shared] Remove telemetry prefixes

* [shared] remove types.ts

Opionionated change: it was only being used for IFlashMessage, and at this point I think it's more useful to go in one level deeper to grab the type you need

* [server] remove route dependencies prefixes

* [server] Various type cleanups

- plugin.ts - remove unnecessary export
- MockRouter - remove prefix for request type, change IMockRouter to match Kibana's IRouter
- check_access - remove prefixes
- callEnterpriseSearchConfigAPI - remove prefixes
- EnterpriseSearchRequestHandler - remove prefixes

* [common] Remove InitialAppData prefix

+ remove unnecessary export from public/plugin.ts

* [common] Remove Meta prefixes

* [common] Remove configured limits prefixes

* [AS] Remove Account and Role prefixes

* [AS] Remove Engine prefixes

* [AS] Remove credentials prefixes

* [AS] Remove log settings prefixes

* [WS] Remove account/organization/initial data prefixes

* [WS] Remove group(s), user, & content source prefixes

+ GroupLogic and GroupsLogic refactor - remove unnecessary defs in actions, it's already defined in the Actions interface above and in some cases (e.g. old History param) is causing out of date issues

* [WS] Misc type fixes

- TSpacerSize -> SpaceSizeTypes
- SourcePriority - remove prefixes
- IComponentLoader - this isn't used anywhere else and appears to be component props so it probably should live only within component_loader.tsx
- Remove recent feed activity prefix

* [WS][Opinionated] Move interfaces not used in server/ out of common/ and to public/

* Fix recently rebased types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants