[ES UI Shared] Added README

[jloleysens]

Summary

Per title.

Additional Context

See comment on file changes: https://github.com/elastic/kibana/pull/70903/files#r455637115

[elasticmachine]

Pinging @elastic/es-ui (Team:Elasticsearch UI)

[sebelga]

Thanks for writing these guidelines @jloleysens ! overall looks great, I added a few comments, let me know your thoughts 👍

[sebelga]

"...to public or server code (or both) ..."?

[sebelga]

Maybe add that the name of the folder should match the name of the package that will be exported. So math will be Math and index_managemnt will be IndexManagement?

[sebelga]

Maybe give the example with the filename too?

Write your function (with accompanying tests) in ./__packages_do_not_import__/math/calculate_fibonacci.ts.

[sebelga]

Maybe here be more specific as we already export multiple modules?

Add the Math module to the list of exported modules in ./public/index.ts, e.g. export { <...other modules>, Math }

[cjcenizal]

I understand the intention behind requiring the use of namespaces, but I don't think naming collisions will be a problem for us. In my experience naming collisions are rare and usually occur when two separate codebases are managed by separate teams that are unaware of one another. In our case, we're maintaining the entire codebase so I think we'll be able to anticipate and avoid any naming collisions that arise.

There are existing examples of large module surface areas that don't have naming collisions, e.g. the 65 service functions/variables exported by EUI. So I think there's some reassuring precedent.

That said, I'm comfortable moving forward with the structure we currently have if everyone likes it (disagree and commit for the win!).

I'm approving the PR to unblock progress, but I would really like @yuliacech to review this when she gets back next week. As the freshest member of the team, I think she's in the best position to suggest ways to improve these kinds of docs.

[sebelga]

we'll be able to anticipate and avoid any naming collisions that arise.

@cjcenizal The problem is not that we won't be able to solve collisions issues. As you say, we are maintaining the entire code base. The problem is that a package has to be independent and be able to give names in its context without having to tweak and prepend/append suffixes to its name to distinguish from other packages. (e.g. interface Validation {} vs interface FormHookValidation {} so we don't collide.

The example you give from EUI is slightly different. In our case, we have a much broader scope (Forms, Monaco, Http Request, reusable UI components, utils services...) that will keep on growing.

[jloleysens]

@cjcenizal Thanks for the feedback!

I tend to agree that avoiding name collisions is a more theoretical bonus in some cases. I can see its usefulness quite directly for functionality in domains like Ace and Monaco. For the example of the 65 shared services and values from EUI I see namespacing is being used for keys. So the question here might not be to-namespace-or-not-to-namespace, but rather in which instances is it useful? Simultaneously, what is the benefit to having everything done in the same way?

At this point my vote is to create namespaces for functionality exported from es_ui_shared as the rule and that there are may be exceptions. Mainly for simplicity, happy to hear if others feel otherwise regarding simplicity here!

A different benefit of namespacing I was thinking of recently, is colocating origin/domain with functionality (specifically function names). I find it is helpful to see Namespace.functionName() at call site. It tells the reader at that point in the code where this functionality comes from (domain) and provides additional context about the meaning of functionName. For example, Monaco.useXJsonMode() tells me I am using XJSON mode, but also that it is intended to be used in combination with Monaco. I can imagine this to be true of other instances too. The export of es_ui_shared will become a kludge of function names and other values that will have their origin/domain obfuscated.

This point 👆🏻 goes against a common practice in our JS/TS code to destructure. If we avoid destructuring in this case, I think we have another benefit to readability with namespaces.

[sebelga]

The export of es_ui_shared will become a kludge of function names and other values that will have their origin/domain obfuscated.

+++

If we avoid destructuring in this case, I think we have another benefit to readability with namespaces.

I agree. And I also think that if we do deconstruct right on top of the component for example we still have the context right there in the file.

[cjcenizal]

To clarify, I'm against the requirement of namespaces. If someone thinks a module's design benefits from a namespace, then I think they should feel free to use one.

FWIW, core doesn't use require the use of namespaces and exports a broad range of concerns. Just another couple data points: public, server. Not saying we should blindly copy them. But we could ask the Platform team about how they made this decision and if they have any regrets.

[yuliacech]

minor grammar note: "If one wanted to ... one would do the following ..."

[yuliacech]

minor grammar note: "to be dependent" or "to depend".
Also I feel like a word is missing here? "to be dependent on externally <missing_word> in unintended ways"

[yuliacech]

Hi @jloleysens , thanks a lot for the detailed write up for es_ui_shared folder! I especially appreciate the Example section with step-by-step instructions and all the explanations. If possible, could you please add a couple of words about common and static folders? I was looking at those right now, but it was not really obvious for me, what would go there.
About the namespaces: Personally, I prefer having less exports in the top level ./public/index.ts that group the functionality in logical domains, like export { Monaco, Forms, ace }, rather than exporting several functions/classes, like

export {
  installXJsonMode,
  XJsonMode,
  ElasticsearchSqlHighlightRules,
  addXJsonToRules,
  ScriptHighlightRules,
  XJsonHighlightRules,
  collapseLiteralStrings,
  expandLiteralStrings,
} from './console_lang';

[sebelga]

In regard to the example of core, this is exactly what I'd like to avoid. Having SavedObjects or Chrome prepend to all their Interfaces to indicate their domain.

Aside, they are already at 368L, I don't think this file is very "scanable".

@yuliacech The "static" folder will be removed and its content will go to the "__packages_do_not_import__" folder. This was a first try at organizing the es_ui_shared static code 😊

[alisonelizabeth]

Thanks for adding this readme @jloleysens! I’m happy with this direction.

[alisonelizabeth]

In regard to the example of core, this is exactly what I'd like to avoid. Having SavedObjects or Chrome prepend to all their Interfaces to indicate their domain.

Aside, they are already at 368L, I don't think this file is very "scanable".

I agree with this, although I also think it might be worth reaching out to the platform to learn about their experience as CJ pointed out.

[cjcenizal]

From a conversation I had this morning with @kobelb re #71566, I gleaned that our code organization could be heading in a direction akin to Java packages or Go packages. If our code organization evolves in this direction then our use of namespaces here might make it easier to align with that direction.

[cjcenizal]

I had a chat with @stacey-gammon today about the future of code-sharing in Kibana, based off #71566 (comment). She'd like generalized code for building plugins and apps to be shared only through the App Arch or Platform team and not through domain-specific teams (like ES UI), out of concerns for stability and maintenance. I think we can support this by making it clear that the es_ui_shared code isn't meant for use beyond our team's plugins. Can we add a note to the README to make that clear?

[sebelga]

She'd like generalized code for building plugins and apps to be shared only through the App Arch or Platform team and not through domain-specific teams (like ES UI), out of concerns for stability and maintenance.

I can understand the concerns. At the same time, I think we might want to have some guidance on how to promote reusable solutions we build. We have many packages that could be useful to other teams that might not fall in the "for all teams" category (like we saw for the form lib).

If we look at the last reusable component I added, the GlobalFlyout, it would be a waste of resources to have other teams build their own solution for the same problem. Or simply not know about it.

making it clear that the es_ui_shared code isn't meant for use beyond our team's plugins.

What does this mean for the siem team consuming the form lib?

@cjcenizal @stacey-gammon

[EDIT] To make my question clearer: what would be the protocol to move some "es_ui_shared" code out of that folder and to some other (?) folder and say: "hey, we've built this reusable solution, the API is stable and it is fully tested and documented. It helps us solving X, you might want to try it out if you have the same problem!".

[EDIT 2]: @cjcenizal I just read #71566 (comment), If we can define a formal path for sharing code more broadly to test its generalizability we can get rid of es_ui_shared.. I think we are aligned! 😊

[cjcenizal]

My understanding is that Stacey is defining and documenting this process for sharing generalized code, so we don't have it yet but we will at some point. In the meantime, I think we're sort of in a holding pattern when it comes to sharing code. So I suggest that we:

• Continue supporting whoever is consuming our shared code already (e.g. siem)
• Continue developing code with consideration for sharing it in the future
• Halt the addition of any new consumers of our shared code until Stacey's work is done and we have a formalized process in place (by making this clear in the README)

[jloleysens]

Thanks all for the feedback! This was really good information for us all to see :).

I will update the README with a note that we are still developing a pattern for sharing code across Kibana and that this is intended primarily for use within the ES UI team.

[spalger]

Looks like the outdated check is ignoring PRs which it thinks are just docs changes... no good

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 72034 or prevent reminders by adding the backport:skip label.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 72034 or prevent reminders by adding the backport:skip label.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 72034 or prevent reminders by adding the backport:skip label.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 72034 or prevent reminders by adding the backport:skip label.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 72034 or prevent reminders by adding the backport:skip label.