Field Formats namespace by lizozom · Pull Request #56975 · elastic/kibana

lizozom · 2020-02-06T13:42:11Z

Summary

This PR demonstrates a proposed resolution for #52374, trying to balance code readability, API discoverability, DX and documentation generation using ApiExtractor.

The solution involves:

Exporting types directly from index.ts, with a prefix (consistent with core)
Exporting utility functions and constants using a "namespace" (A simple object).

In this PR I applied this approach to fieldFormats:

fieldFormats.FieldFormat ⇒ IFieldFormat
fieldFormats.FieldFormatsRegistry ⇒ FieldFormatsRegistry
fieldFormats.ContentType ⇒ FieldFormatsContentType,
fieldFormats.GetConfigFn ⇒ FieldFormatsGetConfigFn,
fieldFormats.IFieldFormatConfig ⇒ FieldFormatConfig,
fieldFormats.IFieldFormatId ⇒ FieldFormatId

Other utilities are available, as before, on the fieldFormats namespace and documentation passes for this sub-service.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-06T13:42:16Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lukeelmers

Since we are composing the static utils namespaces as objects now, I wonder if the static.ts file convention is even useful anymore?

Plus, what if we have a situation where there are items that need to live in a namespace that don't live in a common directory? It's possible for other services we may have utils that, for example, rely on specific browser APIs that would only be exported under the namespace from public.

I think what I'd propose in this case is simply building the namespace from the public/index and server/index files. This has a few benefits:

Lets you combine utils from common and public/server, solving the issue described above
It's one less level of indirection
When you look at the index file you will immediately see everything that's publicly exported, less confusion over what is/isn't public.

Would look something like this:

// data/public/index.ts
import { FieldFormat, BytesFormat, ColorFormat, etc } from '../common';
import { BrowserThing } from './field_formats';

// types
export {
  FieldFormatsGetConfigFn,
  FieldFormatConfig,
  FieldFormatsRegistry,
  ...etc
} from '../common/types';

// utils
export const fieldFormats = {
  FieldFormat,
  BytesFormat,
  ColorFormat,
  BrowserThing,
  ...etc
};

lukeelmers · 2020-02-06T16:28:38Z

src/plugins/data/common/field_formats/static.ts

-/**
- * Everything the file exports is public
- */


Removing this implies that this file now exports some things that are internal - is that the case?

Well, it exports just one thing, and we don't import * from it anymore.
So I found the comment irrelevant.

My overall vote would be to consider the direction I mentioned about composing these objects directly in the public/index and server/index files, in which case it doesn't matter.

That said, if we were to keep this implementation with static.ts I would think it would be important to identify somewhere in the file that this entire namespace is exported publicly, so you don't run the risk of someone adding something internal here accidentally.

@lukeelmers I update the PR to constructing the namespace in public\server index.ts.

src/plugins/data/public/index.ts

lukeelmers · 2020-02-06T16:32:20Z

src/plugins/data/server/index.ts

+  // fieldFormats
+  fieldFormats,
+  FieldFormatsGetConfigFn,
+  FieldFormatConfig,
+  FieldFormatsRegistry,


Currently (in both public/index and server/index) we have static exports separated from type exports. To avoid confusion maybe we should stick the namespace with the static exports for the time being?

Once we move everything over to this structure, it should be more intuitive whether something is static or a type, but in the interim it may help make things clearer.

This transition shouldn't take more than a few days, so I don't think it's worth leaving it for later.
What do you think?

If we're planning to make the transition right away, and you think it's only a couple days, then I have no concerns about keeping it as-is. I only brought it up in the event that the data plugin might sit like this for awhile before we come back to clean up the rest.

elasticmachine · 2020-02-06T19:28:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: aa27d88

History

💛 Build #24908 was flaky 8832834

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

…ata/field-formats-namespace

lizozom · 2020-02-10T09:23:22Z

@elasticmachine merge upstream

kibanamachine · 2020-02-10T10:52:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 71e52ce

History

💚 Build #25442 succeeded ce41463
💚 Build #24984 succeeded aa27d88
💛 Build #24908 was flaky 8832834

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

ppisljar

code LGTM

markov00

Changes in kibana-app owned files LGTM

* Field Formats namespace * Export IFieldFormatsRegistry and FieldFormatsRegistry separately. * remove field_formats export from data plugin and adjust lens test * Updated doc return types * Cleanup fieldFormat namespace and define it index.ts Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Field Formats namespace * Export IFieldFormatsRegistry and FieldFormatsRegistry separately. * remove field_formats export from data plugin and adjust lens test * Updated doc return types * Cleanup fieldFormat namespace and define it index.ts Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Field Formats namespace

8832834

lizozom added Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 Team:AppArch v7.7.0 labels Feb 6, 2020

lizozom requested a review from a team February 6, 2020 13:42

lizozom requested a review from a team as a code owner February 6, 2020 13:42

lizozom self-assigned this Feb 6, 2020

lizozom requested a review from lukeelmers February 6, 2020 13:42

lukeelmers reviewed Feb 6, 2020

View reviewed changes

Liza K added 3 commits February 6, 2020 18:51

Export IFieldFormatsRegistry and FieldFormatsRegistry separately.

d1d37ed

remove field_formats export from data plugin and adjust lens test

7ffef62

Updated doc return types

aa27d88

elastic deleted a comment from kibanamachine Feb 6, 2020

Liza K added 2 commits February 9, 2020 15:38

Merge branch 'master' of github.com:elastic/kibana into newplatform/d…

49d09cf

…ata/field-formats-namespace

Cleanup fieldFormat namespace and define it index.ts

ce41463

lizozom requested a review from lukeelmers February 9, 2020 14:19

lizozom added the review label Feb 9, 2020

Merge branch 'master' into newplatform/data/field-formats-namespace

71e52ce

This was referenced Feb 10, 2020

AppArch plugins documentation generation #56881

Closed

moving visualize/utils to new platform #56650

Merged

lizozom added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Feb 10, 2020

ppisljar approved these changes Feb 10, 2020

View reviewed changes

markov00 approved these changes Feb 10, 2020

View reviewed changes

lizozom merged commit 09c6f25 into elastic:master Feb 10, 2020

This was referenced Feb 10, 2020

[7.x] Field Formats namespace (#56975) #57204

Merged

[Discuss] Organising/grouping exports from plugins like kibana_utils #52374

Closed

lizozom mentioned this pull request Feb 12, 2020

Explicit exports for data plugin #57434

Closed

8 tasks

Conversation

lizozom commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 6, 2020

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

lukeelmers Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

lukeelmers Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom Feb 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukeelmers Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

lukeelmers Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 6, 2020

💚 Build Succeeded

History

Uh oh!

lizozom commented Feb 10, 2020

Uh oh!

kibanamachine commented Feb 10, 2020

💚 Build Succeeded

History

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lizozom commented Feb 6, 2020 •

edited

Loading